temmy222 / PyTOUGHREACT

MIT License
6 stars 1 forks source link

JOSS review: documentation improvements #7

Closed frank1010111 closed 5 months ago

frank1010111 commented 7 months ago

This is part of a JOSS review.

I have identified a few issues with documentation. Apologies for the long list.

I haven't gotten to the tutorial yet, so I don't have any comments or suggestions there right now.

temmy222 commented 7 months ago

Hi. I am working on resolving the issues raised

frank1010111 commented 6 months ago

Thank you,

It would also be very helpful for the docs and readme to talk about the TOUGHREACT binaries. I'm guessing you have to at least get the TOUGHREACT binary. But also maybe tough2 for tmvoc?

frank1010111 commented 6 months ago

Okay, I'm getting to the tutorials now. My first question concerns these lines:

https://github.com/temmy222/PyTOUGHREACT/blob/d57a245128f79d3a8a7fdcd866eca1b44f36f146/documentation/tutorial.rst?plain=1#L54-L76

These properties t2react of don't appear to be documented. Are they required? What is the react parameter dictionary? What keys are allowed and what do they mean?

frank1010111 commented 6 months ago

For the grid class part:

https://github.com/temmy222/PyTOUGHREACT/blob/d57a245128f79d3a8a7fdcd866eca1b44f36f146/documentation/tutorial.rst?plain=1#L85-L91

What do the values for rocktype mean? rocktype is documented nowhere I can find, and it appears to be pulled from PyTOUGH, which has limited information on the properties. Things like the units of each measure are missing even from their code.

Then you delete a rock type and add in sand. What is the default rock type? Does it always get deleted?

What are the differences between this grid that you call t2reactgrid, and the Grid object? It took me a while to figure out that they were distinct entities.

temmy222 commented 6 months ago

Hi @frank1010111 . Thanks for your review. I believe I have fixed most of the issues you raised on the documentation. Please let me know if I missed any. I will begin fixing the issues you raised with the tutorials immediately.

blakeaw commented 6 months ago

Hi @temmy222, I'm not sure if you have addressed this already, but I wanted to note that I agree with @frank1010111's comment about insufficient information in most docstrings:

Most of the docstrings provide insufficient information to use the method, class, or function.

I ran into this problem while trying to follow along with the tutorial. I thought maybe the API docs would help, but they are lacking descriptions of the different objects (or have only very short ones). In particular, for each class, I'd recommend describing the object it represents along with some context on its purpose in the simulations.

If you have addressed this already please let me know, as any changes don't seem to be reflected in the latest version of the docs on readthedocs.

temmy222 commented 5 months ago

Hi @frank1010111. Thanks for your review

Here are comments to your first comments on this issue

1) Link to PyTOUGH has been corrected 2) Documentation updated to correct all mentioned points 3) Contributions updated 4) Dependencies same as pyproject.toml 5) License updated to show MIT everywhere 6) Paper content itself is less than 1000 words. The examples are added which make it appear to be more than 1000 words

frank1010111 commented 5 months ago

Hi Temmy,

Thank you for these changes. I am seeing improvement in the documentations.

Unfortunately, there have also been some breakages in the examples. I am trying to get things working again, as documented here: https://github.com/temmy222/PyTOUGHREACT/pull/17

Is this the thddem.dat file that the code seems to need? There doesn't appear to be one in the current repository, so I had to go searching through the history.

temmy222 commented 5 months ago

Thank you for the fix @frank1010111. Yes, that is the required thddem.dat. I removed it from the repo because it seems to have been changed. Users can specify their database themselves