Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(GweInputData): large GWE models throwing stackoverflow error #1999

Merged
merged 7 commits into from
Aug 30, 2024

Conversation

emorway-usgs
Copy link
Contributor

@emorway-usgs emorway-usgs commented Aug 21, 2024

The following two lines:
this%gwerhos = gwerhos
this%gwecps = gwecps

should have instead used:

this%gwerhos => gwerhos
this%gwecps => gwecps

That discovery led to a number of other fixes since the variables in question were pointers and not copies of data. While in GweInputData.f90, updated it to follow latest style guide. Will adjust other GWE src files in the future.

While running the "Barends" model locally with 200,000 nodes, GWE was crashing with a stackoverflow error while trying to copy an entire array into another array in one line.

Amended this PR with a new smoke test that asserts two neighboring models fail when the density of water is specified differently among the neighbors. Currently, there are no methods for averaging values on either side of the interface. So, the new test first runs and verifies that the simulation runs to completion when the density of water is the same in the two neighbors. Next, the same simulation is run except that the density of water is just a little different in the 'right-hand' model.

The new smoke test should be ready to bring into parallel testing once the following is addressed by @mjr-deltares (lines 512-514ish in GweGweConnection.f90 - there is a similar comment in GwtGwtConnection.f90):

    ! we cannot validate the remainder (yet) in parallel mode
    if (.not. gweEx%v_model1%is_local) return
    if (.not. gweEx%v_model2%is_local) return

@mjr-deltares
Copy link
Contributor

Hi @emorway-usgs , that last commit looks a bit different from what we discussed last week. Maybe have a call so I can explain my thoughts?

@emorway-usgs emorway-usgs marked this pull request as draft August 27, 2024 13:25
@emorway-usgs emorway-usgs changed the title fix(GweInputData): large GWE models were throwing stackoverflow error fix(GweInputData): large GWE models throwing stackoverflow error Aug 27, 2024
@emorway-usgs emorway-usgs added bug maintenance Sprucing up the code labels Aug 28, 2024
@emorway-usgs emorway-usgs marked this pull request as ready for review August 28, 2024 22:11
Copy link
Contributor

@mjr-deltares mjr-deltares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I will make a note of the validation not being done in parallel but this can go in.

@emorway-usgs emorway-usgs merged commit 9c38363 into MODFLOW-USGS:develop Aug 30, 2024
22 checks passed
@emorway-usgs emorway-usgs deleted the fix_gweInputData branch August 30, 2024 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug maintenance Sprucing up the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants