Overall, this is a great contribution to understanding physical parameters from geophysical models. From the example notebooks, it is relatively easy to use and seems to produce realistic results.
The code base is missing some important components listed below, that once implemented will make pide a robust open-source package that could have great impact on the community.
Doc-Strings: The code base could use more documentation in the doc-strings specifically describing parameter options and formats of the inputs. See below.
Tests: There is no test suite that I could see. The code base would be much more robust if tests were implemented.
Documentation: Documentation is left to example notebooks. It is strongly recommended to setup a documentation API using something like Sphinx and ReadTheDocs, as there is a skeleton setup in the docs folder. You could easily include your example notebooks in the index.rst
Notebooks: Could set up a Binder or other web-based service to run the notebooks. You just need to run through them and make sure they all run, see below.
Manuscript
This project introduces a code to estimate/calculate/model physical parameters of geophysical model to provide a more physical interpretation to subsurface models. The manuscript is well written and organized. Below are some minor comments.
Minor Comments
Line 9: suggest changing "Python3" to "Python (>3.8)" as require in the setup.py file.
Line 18: suggest changing "Moreover, ..." to "Moreover, pide is built as a modular tool that allows users to contribute new methods and functionality.
Line 28: suggest changing the reference to include the name of the package [MATE](), and [SigMelts]().
Line 40: change pide to pide
Line 43: change "...or how water behave..." to "...or how water behaves..."
Line 56 and 57: change "pide" to pide
In the future should think about using simpeg for forward modeling of electrical and electromagnetic properties.
Not quite sure what you mean in line 60, which program architecture?, also architecture is mispelled.
Figure 1: can yuo increase the font size of the overall figure, its a little difficult to read. Especially the external 3d info circle and any box with sub elements. Also suggest expanding the caption so the figure can stand alone.
Could add a second figure from one of the example notebooks, like from notebook 10 showing a 2D subsurface model that would highlight expected outputs based on the workflow.
Code Base
Installation
using pip install pide on Python 3.9 produces an error when trying install pycifrw
See Issue #3
Documentation
docs: Only includes the JOSS manuscript and a skeleton of a Sphinx build other than that no documentation that could be rendered.
Strongly encourage you to build the docs, you wouldn't have to add much just include your Jupyter Notebooks into something like ReadTheDocs so that user has a reference instead of searching through the code base. See https://docs.readthedocs.io/en/stable/guides/jupyter.html.
method documentation is minimal, but consistently has inputs, some sort of options and an example. Strongly suggest defininig the options as some have shortened names or abbreviations that may not be common to the user. For example in p_object.list_mineral_econd_models define what each ol,opx,cpx,garnet,mica,amp,quartz,plag,kfelds,sulphide,graphite,sp,rwd_wds,perov,mixture,other option is.
Also suggest defining the shape of input arrays for example pide.set_pressure should that be a 1D array or n x m or something else?
Another example is in p_object.set_mineral_conductivity the options in the Jupyter notebook in examples 1_Olivine_Conduction_Mechanisms code in the third box p_obj.set_mineral_conductivity_choice(ol = '4/proton') the option 4/proton is not described in the method documentation. Please update. You could add information in the method list_mineral_econd_models to include the options for each model.
Tests
No tests exists that I could find. Strongly encourage developing a test suite to demonstrate reproducibility and accuracy of calculations.
Examples
Suggest using markdown headings for each section within each notebook so the user could follow an outline.
0_pide_general_tutorial.ipynb It would great if you could add more information about the pide object, like its purpose, what its main functionality is and how it links the other main objects.
1_Olivine_Conduction_Mechanisms.ipynb like above suggest adding the options to each model in list_mineral_econd_models.
2_PhaseMixing.ipynb: good
3_Seismic_Velocity_Calculation.ipynb: good
4_Material.ipynb: good
Suggest adding in the method documentation the form or inputs like el_cond_selections what is the format of the dictionary {'opx':0,'cpx':0, 'ol':4}. Keys are minerals and values are conductivity?
5_Geotherm_and_Geophysical_Anomalies.ipynb: Might add a note at the end of why the resistivity is so large (granite).
6_Inversion.ipynb: See Issue #1
7_Metasomatism_Maps_from_MT.ipynb: See Issue #4
8_MantleWaterContent_and_Rheology_from_MT.ipynb: See Issue #5
9_2D_Underworld_Conversion.ipynb: See Issue #6
10_2D_Underworld_Conversion: suggest adding a description of what's in the H5 files and if a user wanted to create their own how they could create an H5 of similar format. See Issue #2
It would be nice to have an example demonstrating the potential fields because you point them out in the manuscript.
Reproducibility
Once the bugs are fixed in the notebooks as outlined above, and tests are implemented the package will demonstrate reproducibility.
Review
Overall, this is a great contribution to understanding physical parameters from geophysical models. From the example notebooks, it is relatively easy to use and seems to produce realistic results.
The code base is missing some important components listed below, that once implemented will make
pide
a robust open-source package that could have great impact on the community.docs
folder. You could easily include your example notebooks in theindex.rst
Manuscript
This project introduces a code to estimate/calculate/model physical parameters of geophysical model to provide a more physical interpretation to subsurface models. The manuscript is well written and organized. Below are some minor comments.
Minor Comments
setup.py
file.pide
is built as a modular tool that allows users to contribute new methods and functionality.pide
pide
Code Base
Installation
pip install pide
on Python 3.9 produces an error when trying install pycifrwDocumentation
docs
: Only includes the JOSS manuscript and a skeleton of a Sphinx build other than that no documentation that could be rendered.ReadTheDocs
so that user has a reference instead of searching through the code base. See https://docs.readthedocs.io/en/stable/guides/jupyter.html.inputs
, some sort of options and anexample
. Strongly suggest defininig the options as some have shortened names or abbreviations that may not be common to the user. For example inp_object.list_mineral_econd_models
define what eachol,opx,cpx,garnet,mica,amp,quartz,plag,kfelds,sulphide,graphite,sp,rwd_wds,perov,mixture,other
option is.pide.set_pressure
should that be a 1D array or n x m or something else?p_object.set_mineral_conductivity
the options in the Jupyter notebook in examples1_Olivine_Conduction_Mechanisms
code in the third boxp_obj.set_mineral_conductivity_choice(ol = '4/proton')
the option4/proton
is not described in the method documentation. Please update. You could add information in the methodlist_mineral_econd_models
to include the options for each model.Tests
No tests exists that I could find. Strongly encourage developing a test suite to demonstrate reproducibility and accuracy of calculations.
Examples
Suggest running black on your notebooks to get consisten formatting. https://github.com/drillan/jupyter-black
0_pide_general_tutorial.ipynb
It would great if you could add more information about thepide
object, like its purpose, what its main functionality is and how it links the other main objects.1_Olivine_Conduction_Mechanisms.ipynb
like above suggest adding the options to each model inlist_mineral_econd_models
.2_PhaseMixing.ipynb
: good3_Seismic_Velocity_Calculation.ipynb
: good4_Material.ipynb
: goodel_cond_selections
what is the format of the dictionary{'opx':0,'cpx':0, 'ol':4}
. Keys are minerals and values are conductivity?5_Geotherm_and_Geophysical_Anomalies.ipynb
: Might add a note at the end of why the resistivity is so large (granite).6_Inversion.ipynb
: See Issue #17_Metasomatism_Maps_from_MT.ipynb
: See Issue #48_MantleWaterContent_and_Rheology_from_MT.ipynb
: See Issue #59_2D_Underworld_Conversion.ipynb
: See Issue #610_2D_Underworld_Conversion
: suggest adding a description of what's in the H5 files and if a user wanted to create their own how they could create an H5 of similar format. See Issue #2Reproducibility
Once the bugs are fixed in the notebooks as outlined above, and tests are implemented the package will demonstrate reproducibility.