openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
719 stars 38 forks source link

[PRE REVIEW]: DPFEHM: a differentiable subsurface physics simulator #4544

Closed editorialbot closed 2 years ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@omalled<!--end-author-handle-- (Daniel O'Malley) Repository: https://github.com/OrchardLANL/DPFEHM.jl Branch with paper.md (empty if default branch): Version: v0.1.0 Editor: !--editor-->@jedbrown<!--end-editor-- Reviewers: @WilkAndy, @rtmills, @rezgarshakeri Managing EiC: Kevin M. Moerman

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/8d7a69a54acce2406ed6f857501768c8"><img src="https://joss.theoj.org/papers/8d7a69a54acce2406ed6f857501768c8/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/8d7a69a54acce2406ed6f857501768c8/status.svg)](https://joss.theoj.org/papers/8d7a69a54acce2406ed6f857501768c8)

Author instructions

Thanks for submitting your paper to JOSS @omalled. Currently, there isn't a JOSS editor assigned to your paper.

@omalled if you have any suggestions for potential reviewers then please mention them here in this thread (without tagging them with an @). In addition, this list of people have already agreed to review for JOSS and may be suitable for this submission (please start at the bottom of the list).

Editor instructions

The JOSS submission bot @editorialbot is here to help you find and assign reviewers and start the main review. To find out what @editorialbot can do for you type:

@editorialbot commands
editorialbot commented 2 years ago

Hello human, I'm @editorialbot, a robot that can help you with some common editorial tasks.

For a list of things I can do to help you, just type:

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf
editorialbot commented 2 years ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.04 s (1256.8 files/s, 148969.7 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Julia                           36            295            347           2767
TOML                             2            205              1            911
Markdown                         3             50              0            360
Jupyter Notebook                 1              0             96            208
TeX                              1              0              0             51
YAML                             2              1              4             38
-------------------------------------------------------------------------------
SUM:                            45            551            448           4335
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 2 years ago

Wordcount for paper.md is 608

editorialbot commented 2 years ago

:warning: An error happened when generating the pdf.

Kevin-Mattheus-Moerman commented 2 years ago

@omalled thanks for this submission to JOSS. I am an AEiC with JOSS and here to help with the initial steps (and with finding a handling editor). I attempted to fix the compilation error with this PR: https://github.com/OrchardLANL/DPFEHM.jl/pull/8, can you have a look? Once you've implemented the fix you can call @editorialbot generate pdf to attempt paper compilation again. Thanks

Kevin-Mattheus-Moerman commented 2 years ago

@editorialbot check references

Kevin-Mattheus-Moerman commented 2 years ago

@editorialbot invite @jedbrown as editor

@jedbrown is this your cup of tea :tea:, would you be able to handle this as editor? Thanks.

editorialbot commented 2 years ago

Invitation to edit this submission sent!

omalled commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jedbrown commented 2 years ago

This looks interesting and going in a direction that I think is very fruitful. Notes/questions from my brief survey:

  1. I see there are great examples, but the actual package is only 543 LOC. This usually triggers a scope query.
  2. It would be helpful for the paper to describe capabilities in comparison with existing subsurface flow software and any major assumptions in the present model. How difficult would it be to support tensor-valued conductivity or non-orthogonal grids (needs multi-point flux)? How about a different equation of state, perhaps with trainable parameters?
  3. I notice a TODO related to handling capillary pressure.
  4. Are there any performance claims relative to common solvers for such problems?
  5. What is your verification and validation strategy?
omalled commented 2 years ago

Thanks for taking a look. Here's my perspective on the points you raised:

  1. There are a couple of companion packages that were built to support DPFEHM. One is DifferentiableBackwardEuler, which does the time-integration of the ODEs in a differentiable way. The other is NonlinearEquations, which does sparse automatic differentiation, and was intended to reduce the number of lines of code developers need to write to build PDE solvers like these. I don't include them in the DPFEHM repo, since they could be useful in their own right. When I give talks about DPFEHM, I have slides showing that the @NonlinearEquations.equations macros (e.g., here) expand ~20 lines of code describing the equations into ~500 lines of code that compute the residuals and the sparse Jacobians. That's how many lines of code you'd need to write (for each type of physics) doing this the usual way. Overall, this was a lot more than 3 months of work for an individual, which was the criteria I found on the JOSS website.
  2. This is a good point -- I'll add more about comparisons and assumptions in the paper. In principal, it shouldn't be hard to do things like multi-point flux. However, I'm not experienced with those methods and my focus has been on the differentiability angle, so I kept the numerical methods vanilla (two-point flux approximation). I think it should be possible to do a trainable equation of state but we haven't played around with that.
  3. Yes, something I've learned about subsurface flow solvers is you could always add more physics. :smile:
  4. I have done a performance comparison with PFLOTRAN on some flow simulations in discrete fracture networks that a colleague had sitting around (which led to this paper). DPFEHM was giving a pretty big speed-up on those problems (~20X, IIRC). However, I attribute that to using more appropriate linear solvers than my colleagues were using with PFLOTRAN (they were using BiCG with an ILU preconditioner, whereas I was using CG with an AMG preconditioner, IIRC). The place where you'd really get a performance advantage is computing the gradient of a loss function involving a subsurface flow simulation. E.g., this repo includes a model that was part of a big project related to the groundwater at LANL. When that project was going on, we were using FEHM (as opposed to DPFEHM) to do the simulations. To keep the cost of computing the gradients manageable, we had to reduce the number of parameters from the natural parameterization (1 permeability per grid node, so O(10^6) parameters) to something much smaller, like O(100) parameters. Even with the reduction in the number of parameters, to compute that gradient in a reasonable time we had to use hundreds of CPUs. With DPFEHM, we could compute that gradient in about the same amount of time on a laptop. We could also compute the gradient of the natural parameterization (with O(10^6) parameters) on a laptop in about the same amount of time. So, that's where the real advantage is. In the jargon of computational science, it's the difference between using the adjoint method to compute the gradient (fast) and using finite differencing to compute the gradient (slow). I'll add some discussion to the paper along these lines.
  5. There are a number of tests that we've developed to ensure we don't break things. E.g., comparing to things like the Theis solution and the Thiem solution. I've separately done tests to compare with PFLOTRAN on the discrete fracture networks I mentioned earlier. I've also done more of a validation using the example here, which comes from a real-world problem. I don't include these more complex tests in the repo's test set, because they require a lot of supporting data and are too slow to test quickly. Still, they've helped build confidence that the models are doing the right thing, even for complex problems.
omalled commented 2 years ago

Regarding point 1, I'd also note these earlier packages, which are basically prototypes for DPFEHM and NonlinearEquations: FiniteVolume and LinearAdjoints. They explore some of the same ideas, but are not as full-featured. E.g., DPFEHM has a lot more physics than FiniteVolume and LinearAdjoints works for linear equations, whereas NonlinearEquations deals with, as you would guess from the name, nonlinear equations.

omalled commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jedbrown commented 2 years ago

@editorialbot assign @jedbrown as editor

editorialbot commented 2 years ago

Assigned! @jedbrown is now the editor

jedbrown commented 2 years ago

@omalled Would you be willing to set a github action to run your tests?

jedbrown commented 2 years ago

:wave: @WilkAndy @rtmills @rezgarshakeri :wave: Would you be available to review this submission for JOSS?

rezgarshakeri commented 2 years ago

wave @WilkAndy @rtmills @rezgarshakeri wave Would you be available to review this submission for JOSS?

Yes, I am available.

WilkAndy commented 2 years ago

Yes, i am available. However, i'm pretty sure i've just got covid, so may be out-of-action during the next week or so. Some initial comments on the manuscript:

  1. Perhaps "subsurface physics simulator" is an exaggeration? Eg, where is the geochemistry, solid mechanics, geophysics? There's nothing shameful with something like "groundwater simulator".
  2. I was initially confused by "differentiable" and "automatic differentiation". Initially i thought you meant differentiation of a complicated equation of state, eg, d(viscosity)/d(temperature), especially concentrating on the region around a phase transition (liquid to gas)! I think it'd be helpful to include the word "adjoint" somewhere, and mention an example of d(head)/d(conductivity).
  3. I think the "statement of need" should be expanded by a few sentences. Your examples of CO2 sequestration, inverse analysis and uncertainty quantification are tantalizing, but another sentence or two, explaining exactly why and how the deriviatives are needed and used in these cases would make the article much clearer.
  4. Similar to previous, it would be really cool to see a machine-learning example fully (or even partially) fleshed-out on your webpage. At the moment, a casual reader might think "i can easily solve Theis for a heterogeneous aquifer, so what's the big deal?"
  5. The "...used in several publications including one citation" seems a bit strange - either cite them all, or just say "used in one citation".
  6. I think the PEST software should be mentioned, compared, contrasted. https://pesthomepage.org/ . In Australia at least, this is almost universally used by groundwater people, and seeing a few short pros/cons might encourage readers to give your software a try.
omalled commented 2 years ago

Thanks, @WilkAndy, for the quick feedback and I hope you get well soon! Here's my perspective:

  1. The name is a little tough. In the early days, I called it a "subsurface flow simulator", then a "subsurface flow and transport simulator" after I added transport. Recently, I went with "subsurface physics simulator" to cover the wave equation simulator. I agree it could be seen as overly broad. Of course, I don't mean to imply it could simulate all possible subsurface physics. But, it does do flow, transport, and the wave equation. I'm not sure what a good umbrella for that is other than "subsurface physics". I am open to suggestions.
  2. This is a good observation -- I'll clarify this.
  3. Will do.
  4. We have an example of that here, which the README.md points to in the "advanced usage" section. Still, I agree it would be better to expand the advanced usage section a bit to demonstrate the capabilities more thoroughly. I will do that.
  5. I must have messed up the formatting of the citations. There are supposed to be 3 there. I'll fix that. Thanks for pointing this out.
  6. I believe PEST is in a different category. My understanding is that PEST is used to do things like inverse analysis, sensitivity analysis, UQ, etc. given an arbitrary numerical model. By contrast, DPFEHM is the numerical model itself. I could add a discussion of PEST, if desired. Something along the lines of "PEST takes a non-intrusive approach to UQ, etc., which allows it to work with any simulator but forces it to treat the simulator as a black box. DPFEHM lays the groundwork for next-generation UQ tools that utilize the gradient and Jacobian information that DPFEHM efficiently provides." Would something like that help?
WilkAndy commented 2 years ago

That all sounds good, @omalled . I'm not sure about (1) ! I won't be able to download, test, play-with your code until next week, sorry, so won't complete my checklist till then.

jedbrown commented 2 years ago

Thanks @WilkAndy. Wishing you a speedy recovery. Your timeline is entirely fine.

jedbrown commented 2 years ago

@editorialbot add @WilkAndy as reviewer @editorialbot add @rezgarshakeri as reviewer @editorialbot add @rtmills as reviewer

editorialbot commented 2 years ago

@WilkAndy added to the reviewers list!

jedbrown commented 2 years ago

Ah, I need to issue one command at a time.

@editorialbot add @rezgarshakeri as reviewer

jedbrown commented 2 years ago

@editorialbot add @rtmills as reviewer

editorialbot commented 2 years ago

@rtmills added to the reviewers list!

jedbrown commented 2 years ago

@editorialbot add @rezgarshakeri as reviewer

editorialbot commented 2 years ago

@rezgarshakeri added to the reviewers list!

jedbrown commented 2 years ago

@editorialbot start review

editorialbot commented 2 years ago

OK, I've started the review over in https://github.com/openjournals/joss-reviews/issues/4560.