devito icon indicating copy to clipboard operation
devito copied to clipboard

Examples: Add Darcy flow example

Open sashaowen opened this issue 1 year ago • 7 comments

This PR adds a notebook showing an example of mapping permeability fields to pressure flow using the Darcy flow equation, which is a single-phase porous media flow equation.

sashaowen avatar Sep 06 '22 16:09 sashaowen

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Thanks @ sashaowen for the verynice addition to the tutorials. I left a few comments but it shouldn't be too much to fix.

We usually run all the tutorials through the CI, so it would be good to

  • FIx the random seed so that the result is always the same
  • Add an assert on the norm of the result so that we know it is always running correctly
  • add the #NBVAL_IGNORE_OUTPUT to cells generating figures or such to avoid CI errors.

let us know if you need some help with it.

mloubout avatar Sep 06 '22 17:09 mloubout

Hi, thanks for your replies, I have made the following changes:

  • converted the GRF class to numpy and removed torch from imports
  • Included three samples instead of one
  • fixed the equation
  • fixed a random seed
  • added an assert on the norm of the results
  • added #NBVAL_IGNORE_OUTPUT to the cells generating figures Thanks!

sashaowen avatar Sep 09 '22 10:09 sashaowen

Thanks @georgebisbas, the latest commit should have the changes mentioned in your comments :-)

sashaowen avatar Sep 11 '22 12:09 sashaowen

Left some extra minor comments but is almost there, thanks for the patience.

Please rebase on top of master and try to follow commit message guidelines: https://github.com/devitocodes/devito/wiki/Tags-for-commit-messages-and-PR-titles

mloubout avatar Sep 12 '22 13:09 mloubout

Left some extra minor comments but is almost there, thanks for the patience.

Please rebase on top of master and try to follow commit message guidelines: https://github.com/devitocodes/devito/wiki/Tags-for-commit-messages-and-PR-titles

Thanks for your comments!, Hopefully I have followed the commit guidelines properly this time, apologies for the previous ones. I have made the changes mentioned in your comments, however I'm unsure what rebase on top of master means, could you please point me to a resource on how to do it? Thanks!

sashaowen avatar Sep 12 '22 16:09 sashaowen

Sorry for the delay. rebase lets you rewrite your commit history and make sure that your branch is up to date with master and is mergeable.

You can simply run git rebase -i origin/master (ou'll need your fork to be up to date on master) and it will pop up an editable message with guidelines on what to do.

mloubout avatar Sep 20 '22 17:09 mloubout

Hi @mloubout I think I have addressed your comments, would you mind taking another look when you get the chance please? Hopefully I have done it right!

sashaowen avatar Oct 06 '22 12:10 sashaowen

Hi. It looks good now. Your branch is out of date with master and need to be rebased and will be good to go https://www.atlassian.com/git/tutorials/rewriting-history/git-rebase#:~:text=What%20is%20git%20rebase%3F,of%20a%20feature%20branching%20workflow.

mloubout avatar Oct 06 '22 12:10 mloubout

Codecov Report

Merging #1998 (88bb3c7) into master (88bb3c7) will not change coverage. The diff coverage is n/a.

:exclamation: Current head 88bb3c7 differs from pull request most recent head d2706a5. Consider uploading reports for the commit d2706a5 to get more accurate results

@@           Coverage Diff           @@
##           master    #1998   +/-   ##
=======================================
  Coverage   87.77%   87.77%           
=======================================
  Files         214      214           
  Lines       36984    36984           
  Branches     5570     5570           
=======================================
  Hits        32464    32464           
  Misses       4001     4001           
  Partials      519      519           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar Oct 06 '22 12:10 codecov[bot]

Hi. It looks good now. Your branch is out of date with master and need to be rebased and will be good to go https://www.atlassian.com/git/tutorials/rewriting-history/git-rebase#:~:text=What%20is%20git%20rebase%3F,of%20a%20feature%20branching%20workflow.

I think this is up to date now? + rebased, I think, thankyou for your patience!!

sashaowen avatar Oct 06 '22 12:10 sashaowen

Thanks, all good indeed. Looks liek the assert is failing in the notebook though

----> 1 assert np.isclose(LA.norm(output1),1.0335084)
      2 assert np.isclose(LA.norm(output2),1.3038709)
      3 assert np.isclose(LA.norm(output3),1.3940924)

Maybe a tolerance would make it more stable , for example np.isclose(a, b, atol=1e-4, rtol=0) should be ok usually

mloubout avatar Oct 06 '22 13:10 mloubout

@mloubout Added a tolerance and seemed to run fine locally so hopefully that should work now :)

sashaowen avatar Oct 06 '22 13:10 sashaowen

@mloubout still appear to be errors with the assert, but I'm not sure why this is, when I run it in Vs code from github desktop it works?

sashaowen avatar Oct 06 '22 15:10 sashaowen

I just ran the same code separately in a jupyter server compared to VS code and got slightly different numbers, i.e

output 1 in VS code: 1.0335084

output 1 in a Jupyter server: 1.0335636

running the assert in the Jupyter server failed for 1e-4 but worked for 1e-3, I could try changing to 1e-3 if that is acceptable?

sashaowen avatar Oct 06 '22 16:10 sashaowen

Yes

mloubout avatar Oct 06 '22 16:10 mloubout

committed with 1e-3.

sashaowen avatar Oct 06 '22 16:10 sashaowen