search

openjournals / joss-reviews

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

[REVIEW]: DPFEHM: a differentiable subsurface physics simulator #4560

Closed editorialbot closed 1 year 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.1 Editor: !--editor-->@jedbrown<!--end-editor-- Reviewers: @WilkAndy, @rtmills, @rezgarshakeri Archive: 10.5281/zenodo.8329952

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)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@WilkAndy & @rtmills & @rezgarshakeri, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @jedbrown know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Checklists

📝 Checklist for @WilkAndy

📝 Checklist for @rezgarshakeri

📝 Checklist for @rtmills

editorialbot commented 2 years ago

Hello humans, 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 (1270.2 files/s, 149223.6 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Julia                           36            295            347           2767
TOML                             2            205              1            911
Markdown                         3             51              0            367
Jupyter Notebook                 1              0             96            208
YAML                             3              1              5             92
TeX                              1              0              0             58
-------------------------------------------------------------------------------
SUM:                            46            552            449           4403
-------------------------------------------------------------------------------

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

Wordcount for paper.md is 758

editorialbot commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- None

MISSING DOIs

- 10.2172/1657092 may be a valid DOI for title: Amanzi–ATS: Modeling Environmental Systems across Scales [Brief]
- 10.1029/2021wr031188 may be a valid DOI for title: A Comparison of Linear Solvers for Resolving Flow in Three-Dimensional Discrete Fracture Networks
- 10.1615/jmachlearnmodelcomput.2022042093 may be a valid DOI for title: Inverse analysis with variational autoencoders: a comparison of shallow and deep networks
- 10.21203/rs.3.rs-1782030/v1 may be a valid DOI for title: Physics-informed machine learning with differentiable programming for heterogeneous underground reservoir pressure management
- 10.2172/1168703 may be a valid DOI for title: PFLOTRAN user manual: A massively parallel reactive flow and transport model for describing surface and subsurface processes

INVALID DOIs

- None
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

@WilkAndy @rezgarshakeri @rtmills :wave: Welcome to JOSS and thanks for agreeing to review! The comments from @editorialbot above outline the review process, which takes place in this thread (possibly with issues filed in the DPFEHM repository). I'll be watching this thread if you have any questions.

The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention this issue so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

We aim for reviews to be completed within a month or two. Please let me know if you require some more time. We can also use editorialbot to set automatic reminders if you know you'll be away for a known period of time.

Please feel free to ping me (@jedbrown) if you have any questions/concerns.

omalled commented 2 years ago

BTW, I added a GitHub action to run the tests.

WilkAndy commented 2 years ago

Review checklist for @WilkAndy

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

WilkAndy commented 2 years ago

Cannot confirm contributions of Wu Hao and Dylan Harp

WilkAndy commented 2 years ago

@omalled - is it true that the code coverage is about 50%?

omalled commented 2 years ago

Regarding Wu Hao and Dylan Harp, I got the author list by including the people who made a commit. If I do git log | grep Ha, I see Dylan Harp and Wu Hao on there. I haven't checked the code coverage.

rezgarshakeri commented 2 years ago

Review checklist for @rezgarshakeri

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

WilkAndy commented 2 years ago

Hi @omalled - i'm not sure whether you think my open issues at DPFEHM are silly (no offense taken - they're just suggestions). Nothing has happened in that repo for the last month. Or maybe you're all on holiday?

omalled commented 2 years ago

@WilkAndy No, your suggestions are good. 😄 I've just been busy. I'm planning to get back to this next week.

omalled commented 2 years ago

@WilkAndy, I addressed the issues raised here, following the outline here. I also looked into all the issues raised in the repo and got the code coverage up over 85% (seems to not count code that is run at compile time, so the actual coverage is significantly higher, I believe). Got it done "next week" with a couple of hours to spare in my time zone. 😄 Thanks for looking into all this stuff - definitely helping me whip this code into shape!

WilkAndy commented 2 years ago

Hey everyone (viz @omalled ) - this seems to have gone off the boil. I'm quite keen for this article to be published, but have people lost interest?

omalled commented 1 year ago

I'm working on it.

omalled commented 1 year ago

Alright, I think I addressed @rezgarshakeri's comments about documentation.

jedbrown commented 1 year ago

Hi, all. :wave: Where does this stand? @omalled Is it ready in your opinion or are there reviewer comments yet to be addressed?

omalled commented 1 year ago

I think it is ready and I believe I addressed all the comments. Of course, happy to do more if people have other suggestions.

jedbrown commented 1 year ago

@rezgarshakeri @WilkAndy Could you please take a look at the remaining items on your checklist?

rtmills commented 1 year ago

Review checklist for @rtmills

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

rtmills commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

omalled commented 1 year ago

@jedbrown I don't want to be pushy, but wanted to ping this thread and hopefully get it back on people's radar.

WilkAndy commented 1 year ago

@jedbrown I don't want to be pushy, but wanted to ping this thread and hopefully get it back on people's radar.

You are patience and politeness incarnate, @omalled . I plan to issue my final (hopefully!) comments within a few hours.

WilkAndy commented 1 year ago

I've completed my review and happy for this to be published, @omalled and @jedbrown

rtmills commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

omalled commented 1 year ago

I think I have addressed all the changes. Please let me know if there's something else I can do to get this over the finish line. Thanks, everyone!

jedbrown commented 1 year ago

Thanks! @rtmills Could you have a look and see if you can check off the last few items in your review?

rtmills commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

rtmills commented 1 year ago

@jedbrown and @omalled, thanks for your patience. The changes to the PDF address my concerns, and I am happy to mark this as essentially ready for publication! I will, however, note that there needs to be some editing (that I don't think needs my further review) to one of the changes to the paper , because it currently reads

"[...] Alternative codes such as Amanzi-ATS (Mercer-Smith, 2020), which use a more advanced mimetic finite difference discretization that supports non-K-orthogonal. Other methods for non-K-orthogonal grids include multipoint flux approximations [...]"

and part of the first sentence is clearly missing.

This is very nice work. Thanks for sharing it with the world in JOSS!

omalled commented 1 year ago

Thanks for your kind words, @rtmills! I fixed that sentence. @jedbrown is there something else I should do or is this ready to go?

jedbrown commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

jedbrown commented 1 year ago

Thanks @omalled and sorry to be slow getting to this. This is good work and I just have a few questions.

  1. I see that the paper mentions "advection-dispersion" and the repository does too, but the code purports to solve $u_t + \nabla\cdot(\symbf v u) - \nabla\cdot(D \nabla u) - Q = 0$, which is an advection-diffusion equation. I think this may be a terminology thing in which $D$ is defined in a solution-dependent way to yield dispersion (different wave numbers travel at different speeds), or just that "dispersion" has been overloaded to mean a different thing. I wanted to check that this use is deliberate.

  2. The paper discusses applying a Jacobian-vector product, but the adjoint of an implicit method requires a solve with the transpose of the Jacobian. Are you using a custom rule to avoid differentiating through the Newton solve and instead only use the transpose-Jacobian at the converged state? Does your implementation rely in assembled matrices?

  3. This uses a FV method, for which the discrete adjoint (as produced by AD) is not a consistent discretization of the continuous adjoint. For example, naive upwind 1D advection on a non-uniform periodic 1D grid has zero row sums, but not zero column sums. While this may be useful to optimize a functional of interest, it tends to suffer from spurious local minima and can lead to non-physical solutions. Production FV codes with adjoints (e.g., FUN3D and SU2 for CFD) discretize the continuous adjoint or make related modifications to avoid these serious problems with the discrete adjoint. Have you experienced similar issues and would the framework be able to handle such techniques?

  4. It looks like you still have missing DOIs. Can you please add a DOI to all references that have one? These may help..

omalled commented 1 year ago

  1. I am using the word "dispersion" in the same sense as these notes, which is typically how it is used in subsurface flow and transport. We use diffusion to refer to the spreading of mass due to velocity fluctuations at the molecular scale. Disperison, by contrast, is used to describe spreading due to velocity fluctuations at the pore scale (or perhaps a larger scale) that are unresolved by the model. In short, the use is deliberate.

  2. Correct, we are using a custom rule to avoid differentiating through the Newton solve and use only the transpose-Jacobian at the converged state. Yes, it relies on assembled Jacobian matrices that are automatically generated.

  3. I have been keeping an eye out for issues like this but haven't actually encountered them. That being said, the stuff I've done with the advection-dispersion equation has all been on a uniform, non-periodic grid. It would be possible to use the basic techniques underlying this code to build a continuous adjoint for the backward pass, if needed.

  4. I think I fixed all the DOIs this time.

Thanks, @jedbrown!

jedbrown commented 1 year ago

@editorialbot check references

editorialbot commented 1 year ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.3133/sir20105169 is OK
- 10.2172/1657092 is OK
- 10.1029/2021wr031188 is OK
- 10.1615/jmachlearnmodelcomput.2022042093 is OK
- 10.1038/s41598-022-22832-7 is OK
- 10.2172/1168703 is OK
- 10.2172/14903 is OK
- 10.5066/F7RF5S7G is OK

MISSING DOIs

- None

INVALID DOIs

- 10.3102/10769986156061 is INVALID
jedbrown commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

jedbrown commented 1 year ago

Looks like you're missing digits in this reference: https://doi.org/10.3102/1076998615606113

Should also protect capitalization for {Bayesian}.

omalled commented 1 year ago

@editorialbot check references

editorialbot commented 1 year ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.3133/sir20105169 is OK
- 10.2172/1657092 is OK
- 10.1029/2021wr031188 is OK
- 10.3102/1076998615606113 is OK
- 10.1615/jmachlearnmodelcomput.2022042093 is OK
- 10.1038/s41598-022-22832-7 is OK
- 10.2172/1168703 is OK
- 10.2172/14903 is OK
- 10.5066/F7RF5S7G is OK

MISSING DOIs

- None

INVALID DOIs

- None
omalled commented 1 year ago

@jedbrown OK, I fixed that DOI.

jedbrown commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

jedbrown commented 1 year ago

Thanks @omalled, and sorry about the delay on my end. At this point could you:

I can then move forward with recommending acceptance of the submission.