[REVIEW]: SMART: Spatial Modeling Algorithms for Reactions and Transport

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@emmetfrancis@Kevin-Mattheus-Moerman<!--end-editor-- Reviewers: @oalii, @vincent-noel, @mbarzegary Archive: 10.5281/zenodo.10019519

Status

Status badge code:

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

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

@oalii & @vincent-noel & @mbarzegary, 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 @Kevin-Mattheus-Moerman 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 @vincent-noel

📝 Checklist for @mbarzegary

📝 Checklist for @oalii

editorialbot commented 1 year 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 1 year ago

Software report:

github.com/AlDanial/cloc v 1.88  T=0.09 s (502.9 files/s, 135029.2 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          17            887           1530           4721
Jupyter Notebook                 6              0           2890           1234
Markdown                         7            113              0            428
YAML                            11             67             34            305
TeX                              2             13              0            208
TOML                             1             19              8             92
INI                              1              0              0             10
Dockerfile                       1              4              1              6
reStructuredText                 1             19             27              3
-------------------------------------------------------------------------------
SUM:                            47           1122           4490           7007
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository

editorialbot commented 1 year ago

Wordcount for paper.md is 1576

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:

Kevin-Mattheus-Moerman commented 1 year ago

@oalii, @vincent-noel, @mbarzegary thanks for your help reviewing this work! This is where the review happens. I kindly ask you to now formally start the review. Follow the instructions above :point_up: to generate a check box list for yourself here to guide you through the process. Let me know if you have any questions. Thanks again!

Kevin-Mattheus-Moerman commented 1 year ago

@emmetfrancis :wave: this is where the review takes place. Please keep an eye out for comments here from the reviewers, as well as any issues opened by them on your software repository. I recommend you aim to respond to these as soon as possible, and you can address them straight away as they come in if you like, to ensure we do not loose track of the reviewers.

vincent-noel commented 1 year ago

Review checklist for @vincent-noel

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the https://github.com/RangamaniLabUCSD/smart?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@emmetfrancis) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines
[x] Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
[x] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
[x] Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

mbarzegary commented 1 year ago

@editorialbot generate checklist

editorialbot commented 1 year ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@editorialbot commands

mbarzegary commented 1 year ago

@editorialbot commands

editorialbot commented 1 year ago

Hello @mbarzegary, here are the things you can ask me to do:


# List all available commands
@editorialbot commands

# Get a list of all editors's GitHub handles
@editorialbot list editors

# Check the references of the paper for missing DOIs
@editorialbot check references

# Perform checks on the repository
@editorialbot check repository

# Adds a checklist for the reviewer using this command
@editorialbot generate my checklist

# Set a value for branch
@editorialbot set joss-paper as branch

# Generates the pdf paper
@editorialbot generate pdf

# Generates a LaTeX preprint file
@editorialbot generate preprint

# Get a link to the complete list of reviewers
@editorialbot list reviewers

mbarzegary commented 1 year ago

Review checklist for @mbarzegary

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the https://github.com/RangamaniLabUCSD/smart?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@emmetfrancis) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines
[x] Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
[x] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
[x] Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

mbarzegary commented 1 year ago

@editorialbot check references

oalii commented 1 year ago

Review checklist for @oalii

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the https://github.com/RangamaniLabUCSD/smart?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@emmetfrancis) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines
[x] Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
[x] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
[x] Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

oalii 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:

mbarzegary commented 1 year ago

I confirm the quality of the software and the effort the author has put in to develop it. The developed package is successful in reproducing the previously published simulations using its simple, yet powerful data structure. Moreover, the software includes nice documentation and a bunch of tutorials covering most of the features of the package. However, I believe some minor concerns should be addressed before accepting the submission, elaborated in a couple of issues I have opened on the software repo (listed above). I hope the authors find the comments useful.

Kevin-Mattheus-Moerman commented 1 year ago

@emmetfrancis can you please get back to @mbarzegary and work on those issues ☝️ thanks

emmetfrancis commented 1 year ago

Thank you @mbarzegary! The comments you included in Issues are very helpful, and we are currently working to address them. The issue with installation was an excellent catch, we have created a new Docker image that includes Jupyter and are working to address some processor-specific issues with Pyvista. We will let you know if we have any follow-up clarifications, thanks!

oalii commented 1 year ago

Dear Authors,

First, sorry for the detail of my response. Secondly, as @mbarzegary I acknowledge the overall quality of your code and documentation ! You put a lot of efforts into it and it is really appreciated. But I struggled with the docker installation, I left an issue with a bit more details. Some potential users (me included) are more familiar with conda environments than docker containers. Would it be possible to provide a conda recipe to install and run you library (This could widen the spectrum of potential users) ? This is just a question, not a request, as I suspect that the available Fenics version for conda might be incompatible. As soon as the docker issue will be fixed, I'll dive into your notebooks.

jorgensd commented 1 year ago

Dear Authors,

First, sorry for the detail of my response. Secondly, as @mbarzegary I acknowledge the overall quality of your code and documentation ! You put a lot of efforts into it and it is really appreciated. But I struggled with the docker installation, I left an issue with a bit more details. Some potential users (me included) are more familiar with conda environments than docker containers. Would it be possible to provide a conda recipe to install and run you library (This could widen the spectrum of potential users) ? This is just a question, not a request, as I suspect that the available Fenics version for conda might be incompatible. As soon as the docker issue will be fixed, I'll dive into your notebooks.

We currently cannot use conda, as we are waiting for a release of FEnicS that includes the work from: https://dl.acm.org/doi/abs/10.1145/3471138. This is something @finsberg and I are working slowly towards (by resolving various minor bugs in DOLFIN). Ref: https://github.com/RangamaniLabUCSD/smart/issues/66

Kevin-Mattheus-Moerman commented 1 year ago

@emmetfrancis can you please provide an update on how you are addressing the comments/issues raised? Thanks.

emmetfrancis commented 1 year ago

Thank you for checking in, we have resolved almost all of the issues posted by @oalii and @mbarzegary. The only issue we have not addressed is the suggestion by @mbarzegary to add performance testing. The suggestion to add a performance test associated with parallelization is a good one, but we are still working to add some parallelization features in future versions of SMART. We will follow up on this issue soon, we appreciate all the feedback and suggestions!

vincent-noel commented 1 year ago

Dear authors,

Congratulations on the great work ! The software is very well done, well documented, tested, and with many examples. Running the examples via the smart-lab docker image is extremely easy and provide a ready-to-use environment to start experimenting right away. As @oalii said, conda compatibility would be best, but these things can take time and I'm sure they will be resolved. Meanwhile providing everything in a docker environment works very well. I also agree with @mbarzegary that even a basic performance test would be a very good addition. I would also like to see a mention of what kind of parallelization SMART supports, which is something users would expect from a "high performance" software.

jorgensd commented 1 year ago

SMART supports parallelism through MPI. A note on the conda/DOLFIN status: I've made three PRs into DOLFIN

https://bitbucket.org/fenics-project/dolfin/pull-requests/565
https://bitbucket.org/fenics-project/dolfin/pull-requests/566
https://bitbucket.org/fenics-project/dolfin/pull-requests/567 that fixes some edge cases of DOLFIN with MeshView (which is used in SMART). These changes has to be merged into DOLFIN before we can make a release of DOLFIN (and then push a new release on conda). This would also slightly change some internal API of SMART to use the new function dolfin.create_meshview instead of dolfin.MeshView.create.

emmetfrancis commented 1 year ago

Hello all, just wanted to provide a quick update. We are working on integrating these fixes with MeshView (mentioned by Jorgen above) into a new release of SMART, which will allow us to include a performance test demonstrating use of parallelization. Once we complete this (looks like next week at this point), this should resolve all of the raised issues. I will update here as soon as we have this new release, really appreciate all of the feedback and suggestions!

emmetfrancis commented 1 year ago

Thank you for your patience while we have assessed some issues with parallelization. At this point, we have found that the problem stems from an issue using MeshView's in parallel in DOLFIN. This bug is not directly related to SMART, but is rather an issue that needs to be addressed in the underlying DOLFIN code. @jorgensd is working on addressing that in DOLFIN. In the meantime, we have added an explanation of the current status of MPI parallelization in SMART within our README file, under "Functionality Documentation":

The current version of SMART is not compatible with MPI-based mesh parallelization; this feature is in development pending a future release of DOLFIN addressing some issues when using MeshViews in parallel. However, SMART users can utilize MPI to run multiple simulations in parallel (one mesh per process), as demonstrated in Example 3 with MPI.

We are hoping we will be able to proceed with this JOSS submission, as the remaining issue depends on DOLFIN and not SMART code. @Kevin-Mattheus-Moerman, if you can let us know your thoughts on the best way to move forward, it would be much appreciated.

emmetfrancis commented 1 year ago

Hello all, just wanted to follow up briefly. @Kevin-Mattheus-Moerman, any updates or anything we can provide to move the process forward? Thank you.

emmetfrancis commented 1 year ago

@Kevin-Mattheus-Moerman Please let us know the current status of this submission, we believe we have addressed the concerns of the reviewers, with the exception of some ongoing issues with parallelization summarized above, which come down to issues in DOLFIN, not SMART.

meg-simula commented 1 year ago

:wave: @Kevin-Mattheus-Moerman could you advise on how to proceed here? Thanks!

Kevin-Mattheus-Moerman commented 1 year ago

@emmetfrancis @meg-simula apologies for the delay, I have been out on annual leave but back now so will look at this now.

Kevin-Mattheus-Moerman commented 1 year ago

@emmetfrancis @jorgensd can you help provide a summary of what issues are still open. Do I understand it correctly that "performance testing" was recommended, but you are at present unable to add that? How far are you from adding that?

@oalii, @vincent-noel, @mbarzegary are you happy with the changes made and the response to your feedback? Some of you have several checkboxes unticked, could you respond with the key points that you feel still need work?

meg-simula commented 1 year ago

@Kevin-Mattheus-Moerman SMART relies on the larger finite element software FEniCS, in particular its parallel support. There are missing features in FEniCS that makes part of SMART lack native MPI support. Thus, that performance issue is orthogonal to SMART. The fix in FEniCS is unclear, and definitely not something we should wait on. Hope this clarifies. Also see Emmets comments above.

vincent-noel commented 1 year ago

I'm happy with the results, and with the answer to my comment about parallelization. The authors did their best, now the problem lies with FEniCS, who have been notified of the issue.

jorgensd commented 1 year ago

@emmetfrancis @jorgensd can you help provide a summary of what issues are still open. Do I understand it correctly that "performance testing" was recommended, but you are at present unable to add that? How far are you from adding that?

@oalii, @vincent-noel, @mbarzegary are you happy with the changes made and the response to your feedback? Some of you have several checkboxes unticked, could you respond with the key points that you feel still need work?

Hello @Kevin-Mattheus-Moerman, thanks for following up. The only remaining issue associated with JOSS review is the "Reproducibility and Functionality" issue from @mbarzegary. We haven't implemented any extra performance tests because of these issues with parallelization, but if there are any specific suggestions from the reviewers on a different performance test they might want to see, we are happy to consider it.

oalii commented 1 year ago

Hi @Kevin-Mattheus-Moerman and authors, I am currently attempting a conference until the end of the week. Is it ok this I resume my reviewing next week ? I still have a few checkbox to tick. Sorry for this delay.

mbarzegary commented 1 year ago

Hi all, I believe all the issues are resolved except the high-performance claim. What if the authors remove the term "high-performance" from the paper and PyPI repo? Due to certain issues in the underlying FE package, it's not possible to test the parallelism and mesh partitioning, so we cannot call the software "high-performance" I think.

oalii 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:

Kevin-Mattheus-Moerman commented 1 year ago

Thanks @emmetfrancis and all for the clarifying comments.

@emmetfrancis do you think you can address @mbarzegary 's point on rewording "high performance". I leave it up to you how you address it. I feel you may not need to remove the terms if you want to keep them. I would however comment on the current issue to alert readers/users that some performance aspects cannot at present be evaluated due to this issue with your dependency. So perhaps you can add wording in the paper and in your README/docs about this.

Kevin-Mattheus-Moerman commented 1 year ago

@emmetfrancis could you clarify if the above issue with FENiCs could be circumvented by using an older version? If so we could perhaps use that on this review.

oalii commented 1 year ago

@Kevin-Mattheus-Moerman and all the authors,

Please, excuse me again for my delay on this review, not the best of time... I finally managed to get back to it and checked all the boxes of my checklist.

Once again, I would like to acknowledge the solid work done on this library ! I enjoyed reviewing it. As soon as it will be published I will strongly suggest to all the students and young engineers within our team to take a good look at it: It is certainly a example of what a scientific computing library should look like these days.

PS : Exploring the code and examples, some (very) minor comments/questions popped in, I put them below.

oalii commented 1 year ago

General questions/remarks:

The precise and explicit definition of units when setting the simulations is very interesting. In many cases, the main quantities of interest have the same units (e.g. time in sec, concentrations in molecules/um^3...). You did not set any default values for these units. Is this a design decision, so that the user must make the consious choice of selecting an appropriate set of units for its problem? Don't you think that adding default values could spare some times and ease prototyping ?
Can your model encompass vector and/or tensor fields ?
- For instance, if I want to model the dynamics of a transmembrane protein sensitive to the curvature of the membrane: I might what to depict the membrane curvature through its extrinsic curvature tensor, Can I add an such ingredient into your simulation framework ?
- Another example: If I am interested in cytoskeleton dynamics, I might need to describe it with a nematic field or at least a vector field accounting for its polarization. Can I use VectorElements to do that?
You mention somewhere (couldn't retrieve where...) That some notebooks might take some time to run (examples 3, 4 and 6 took some time for me): It could be informative for the user to mention this in the introduction of the corresponding notebooks.
A naive question (more for my general knowledge than anything else...) about your FEniCS/dolfin dependency: you rely on the "legacy" version of dolfin, not the latest dolfinx one. If ever, one day, one would want to use your library with FEniCSx/dolfinx, how hard would it be the update?
- Subsidiary question: Why did you chose to rely on the legacy version and not the current "x" version of these frameworks/libraries ?

Some minor comments/remarks/questions on the example notebooks:

example 1:

I don't really understand the final regression test:
- why only on species B ?
- Where does the L2norm_saved come from ?
- What are you assessing ? seems like the sum ofthe squared values of the field B at every DOF. Is that it ?

example 2:

Could be informative to have the law of mass action or/and the differential equation depicting the chemical reaction at stake.

example 3:

Could be information to have the differential system to solve.
typo right above cell 14: Now we repeat this process for other values of cell radii, to test the dependence of steady state calcium concentration on cell size → I think you are refearing to the Aphos concentration, not calcium concentration, no ?
Concerning the comparison with analytical expectation, I assume you are using Eq.9 from the reference paper Meyer et al (2006) ? It could be informative to explicitly write this equation above cell 18.

example 4:

Differential system to solve (eqs. 1 to 4 in initial paper, only eq.3 is given) could again be informative in the initial description of the problem in the first cell.
Assertion error on the last cell:

---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
Cell In[11], line 7
      5 auc_compare = 0.35133404882191754
      6 percent_error = 100*np.abs(auc_cur - auc_compare)/auc_compare
----> 7 assert percent_error < .01,\
      8     f"Failed regression test: Example 4 results deviate {percent_error:.3f}% from the previous numerical solution"

AssertionError: Failed regression test: Example 4 results deviate 0.012% from the previous numerical solution

Example 5:

Warning generated in cell 10:

2023-09-12 09:50:29,161 smart.model_assembly - WARNING - Warning! Pre-integrating parameter j1pulse. Make sure that expressions j1pulse appears in have no other time-dependent variables. (model_assembly.py:660) 

2023-09-12 09:50:29,162 smart.model_assembly - WARNING - Warning! Pre-integrating parameter j1pulse.  Make sure that expressions j1pulse appears in have no other time-dependent variables.   (model_assembly.py:660)

Example 6:

Same warning as in example 5 in cell 8
Warnings on cell 17:

ROOT -2023-09-12 09:56:35,346 FFC - WARNING -   WARNING: The number of integration points for each cell will be: 144 (log.py:153)
ROOT -2023-09-12 09:56:35,346 FFC - WARNING -            Consider using the option 'quadrature_degree' to reduce the number of points (log.py:153)
ROOT -2023-09-12 09:56:35,710 FFC - WARNING -   WARNING: The number of integration points for each cell will be: 144 (log.py:153)
ROOT -2023-09-12 09:56:35,710 FFC - WARNING -            Consider using the option 'quadrature_degree' to reduce the number of points (log.py:153)

emmetfrancis commented 1 year ago

Thank you @oalii for the detailed feedback, much appreciated! I will certainly plan to go through and address/fix the issues in the examples. Regarding your first main questions:

While setting default units would perhaps somewhat improve usability, it immediately runs the risk of the user not correctly simulating some given problem. We styled this aspect of our software in a similar way to VCell (a popular systems biology software). Note that units become especially confusing (and a source of unintentional mistakes) when coupling 2D-3D problems, as we do here. For instance, 3D concentrations are often specified in some version of molarity (moles per liter), whereas surface concentrations are molecules per area. Reactions between surface and volume species must have rates with carefully chosen units to preserve the integrity of the problem.
FEniCS can definitely be used to solve vector-valued problems, but we do not have any built-in support for such problems in SMART right now. A workaround for nematic fields could just be writing the vector components as separate species (cyto_x, cyto_y, cyto_z), but I imagine the associated reactions may be messy to write out. We appreciate the suggestion and will certainly consider supporting vector-valued species in future iterations of SMART. Regarding the curvature sensitive protein, I think this could be captured in our current framework provided one computes the extrinsic curvature over the mesh. In the current version, this might need to be defined as a species that remains constant over the surface and can then be included in the reaction rate for the protein of interest. In future iterations, we hope to include parameters that can vary over the domain.
There are associated notes in the intros to Examples 3 and 6 saying it may take an hour or longer to execute, I can add a similar note for Example 4.
It would take quite a bit of rewriting to build this software off of dolfinx rather than legacy dolfin. We developed this in legacy dolfin where the tools for mixed dimensional systems are more mature (see this paper). This may be a future direction we go as FEniCSx continues to mature, but currently the transition is not planned for SMART.

emmetfrancis commented 1 year ago

@emmetfrancis could you clarify if the above issue with FENiCs could be circumvented by using an older version? If so we could perhaps use that on this review.

There is no version of FEniCS that would support parallelization for our software currently. @jorgensd can provide a more detailed explanation, as he has been the one working on fixing this parallelization issue in the FEniCS source code.

jorgensd commented 1 year ago

@emmetfrancis could you clarify if the above issue with FENiCs could be circumvented by using an older version? If so we could perhaps use that on this review.

There is no version of FEniCS that would support parallelization for our software currently. @jorgensd can provide a more detailed explanation, as he has been the one working on fixing this parallelization issue in the FEniCS source code.

The issue relates to a corner case in the MeshView implementation from https://dl.acm.org/doi/abs/10.1145/3471138 which is used under the hood of SMART to solve PDEs on sub-domains and interfaces (and represent quantities on these). To the best of my knowledge, the following corner case has never worked as it should with MeshView: A process having a vertex, but not having an entity of the MeshView in question (can happen if an interface aligns with a process boundary). Trying to resolve this, and other parallel issues popping up during benchmarking with SMART, I made several fixes to the core code in DOLFIN, Ref: https://bitbucket.org/fenics-project/dolfin/pull-requests/563 https://bitbucket.org/fenics-project/dolfin/pull-requests/565 https://bitbucket.org/fenics-project/dolfin/pull-requests/567 I've not found a nice way of fixing the corner case mentioned above yet. The corner case mentioned above does not happen on every parallel run; it depends on the number of process used for any current grid.

jorgensd commented 1 year ago

A naive question (more for my general knowledge than anything else...) about your FEniCS/dolfin dependency: you rely on the "legacy" version of dolfin, not the latest dolfinx one. If ever, one day, one would want to use your library with FEniCSx/dolfinx, how hard would it be the update?
* Subsidiary question: Why did you chose to rely on the legacy version and not the current "x" version of these frameworks/libraries ?

To add even more context to this. The development of this library started back in 2019. DOLFINx reached a state the core developers thought was mature in 2022, which is when it was recommend to start new projects in DOLFINx: https://fenicsproject.discourse.group/t/the-new-dolfinx-solver-is-now-recommended-over-dolfin/8249 As @emmetfrancis stated, the current work rely on the MeshView works of Daversin et al, and this is currently in development in DOLFINx in: https://github.com/FEniCS/dolfinx/pull/2271 but has at the time of writing not gotten merged into the main part of the code.

emmetfrancis commented 1 year ago

Hello @oalii, thanks again for the comments. I have been through the examples and just need to make some tweaks/clarifications where you have suggested. However, the assertion error should definitely not occur in Example 4 (I do not get that at least). Can you clarify which version of SMART you installed to help me with troubleshooting this?

oalii commented 1 year ago

@emmetfrancis , Thanks for addressing this issue. I ran the example 4 notebook in the docker image smart-lab:latest:

REPOSITORY                           TAG       IMAGE ID       CREATED        SIZE
ghcr.io/rangamanilabucsd/smart-lab   latest    e42e3f9dfe3a   2 months ago   4.33GB

FYI: I am running this on a MacBookPro M1Pro. Could it be a hardware-related issue ?

emmetfrancis commented 1 year ago

Thanks again, @oalii. It seems likely that the example 4 discrepancy is indeed hardware-related (@finsberg tested on Mac and saw the same thing). We updated the tolerance on the regression test so it should pass for Mac now.

Here are some responses to the smaller concerns with the examples. All are fixed with this PR to SMART. Please let me know if there are any remaining concerns

example 1:

The final regression test is performed by comparing the L2 norm of species B over the domain to a previously stored value (L2norm_saved). I added some text in the notebook to clarify this. Note that we could have chosen a norm of species A, we just need some scalar quantity to compare with previous output.

example 2:

I added the differential equations for easy reference.

example 3:

I added the differential equation for easy reference, fixed the typo and added Eq 9 from Meyer et al for easy reference.

example 4:

I added the differential equation for easy reference.
As mentioned above, the assertion error seems to be hardware specific. We set the tolerance higher so that this example passes for Mac.

Example 5:

Thank you for pointing out this warning; it is certainly confusing and I do not think should be user-facing. The associated concern is actually not applicable to the current version of SMART so I have removed this warning and added some associated details in the doc strings for reference in future development.

Example 6:

Thank you for noting these warnings. They should no longer appear now, I specified a quadrature degree for use in FEniCS (4th degree to be safe).

openjournals / joss-reviews