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

Update postprocessing.py #1329

Closed
wants to merge 1 commit into from
Closed

Conversation

askar-INTERA
Copy link

The upper correction is not necessary, but with the second little correction of adding .flatten() to line 63, the function was returning an indexing error.

The upper correction is not necessary, but with the second little correction of adding .flatten() to line 63, the function was returning an indexing error.
@codecov
Copy link

codecov bot commented Jan 19, 2022

Codecov Report

Merging #1329 (505d79d) into develop (eecd1ad) will decrease coverage by 45.197%.
The diff coverage is 50.000%.

@@              Coverage Diff               @@
##           develop     #1329        +/-   ##
==============================================
- Coverage   74.730%   29.532%   -45.198%     
==============================================
  Files          247       247                
  Lines        53384     52010      -1374     
==============================================
- Hits         39894     15360     -24534     
- Misses       13490     36650     +23160     
Impacted Files Coverage Δ
flopy/mf6/utils/postprocessing.py 9.259% <50.000%> (-88.889%) ⬇️
flopy/mf6/utils/testutils.py 0.000% <0.000%> (-93.334%) ⬇️
flopy/mfusg/mfusgsms.py 6.161% <0.000%> (-91.514%) ⬇️
flopy/modflow/mfevt.py 7.812% <0.000%> (-87.007%) ⬇️
flopy/mf6/utils/lakpak_utils.py 2.097% <0.000%> (-85.315%) ⬇️
flopy/modflow/mfhyd.py 13.223% <0.000%> (-85.124%) ⬇️
flopy/utils/swroutputfile.py 10.714% <0.000%> (-85.079%) ⬇️
flopy/utils/mtlistfile.py 4.620% <0.000%> (-84.700%) ⬇️
flopy/modflowlgr/mflgr.py 6.271% <0.000%> (-84.354%) ⬇️
... and 203 more

@@ -60,7 +60,7 @@ def get_structured_faceflows(
shape = (grb.nlay, grb.nrow, grb.ncol)
frf = np.zeros(shape, dtype=float).flatten()
fff = np.zeros(shape, dtype=float).flatten()
flf = np.zeros(shape, dtype=float)
flf = np.zeros(shape, dtype=float).flatten()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test to show that this is necessary?

Copy link
Author

Choose a reason for hiding this comment

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

test.zip

Please run the two small testing programs, called 1)test_w_modfified_version.py and 2)test_w_nonmodfified_version.py, to see the difference in the output, thanks.

@langevin-usgs
Copy link
Contributor

@askar-INTERA, thanks for submitting this PR. I went ahead and implemented this fix on a separate PR (#1333) and I also added a test. You'll see in #1333 the type of test we are looking for to ensure the fix is correct. We like to add small fast tests that demonstrate working functionality with the code fix. Thanks!

@askar-INTERA
Copy link
Author

Thanks for doing that and sorry that I am not familiar with the type of tests you need as I am still new to GitHub. Excuse my limited knowledge about GitHub but I assume the new separate PR #1333 is totally created by you and will not give me any credit for this little fix, correct?

@langevin-usgs
Copy link
Contributor

Yes, PR #1333 is a new and separate pull request so it will not be attributed to you. However, your PR is documented here and so there is a trail to you. If you can adopt our test requirements and add them to your future PRs, then the fixes will be made in your name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants