Improve documentation and unit tests

[micaeljtoliveira]

Closes #1

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (93f3161) to head (f345c85).

Additional details and impacted files

@@             Coverage Diff             @@
##             main        #4      +/-   ##
===========================================
+ Coverage   95.45%   100.00%   +4.54%     
===========================================
  Files           4         3       -1     
  Lines         198       190       -8     
===========================================
+ Hits          189       190       +1     
+ Misses          9         0       -9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[micaeljtoliveira]

@aekiss @dougiesquire Would you have some time to start looking at the code and documentation in this repo? This would include all the existing code (which is not yet too large), not just the changes in this PR. Any comments and/or suggestions would be more than welcome.

[dougiesquire]

@micaeljtoliveira I had a look through the repo. The code is well written and does what it's supposed to. I'm not sure exactly what you're after in terms of feedback, so I have a bit of a grab-bag of comments. Obviously, please feel free to take or leave as you like:

• Add a README (added in this PR)
• What is the scope of this package? Is it just for reading/writing ACCESS-OM3 configuration files or do you think it will extend beyond that? It's difficult to think about structure and API design without knowing this.
• Are you planning on having the docs and API reference built and rendered somewhere? If yes, is there a tool that can handle your formatting? The tools I’ve used expect markdown/restructuredtext docs and specific formatting of docstrings (googledoc format added in this PR).
• The write_* functions receive a pathlib.Path object, but that’s not very clear from the docstring. I’d suggest updating the docstring or having the option of passing a string (added in this PR).
• Writing MOM_input files mucks up the spacing of the comments which makes them pretty difficult to read. I don't know whether this is worth fixing.
• There’s some redundancy in the name of modules and their methods (e.g. read_mom6_input inside mom6_input). It may be nice to expose the read_* and write_* methods in the top-level __init__.py, so that instead of

  from om3utils.mom6_input import read_mom6_input
  

  I could do

  from om3utils import read_mom6_input
  

  But, as above, the best structure depends on what else might be added to the package.

[dougiesquire]

Just realised my review doesn't include the changes in this PR, which make at least some of my comments redundant. Will update tomorrow

[dougiesquire]

I've updated my comments above. Did you want someone to also look at the profiling code added in #3?

[micaeljtoliveira]

@dougiesquire Thanks for having a look!

Did you want someone to also look at the profiling code added in #3?

I think it's better to first get this one merged.

What is the scope of this package? Is it just for reading/writing ACCESS-OM3 configuration files or do you think it will extend beyond that? It's difficult to think about structure and API design without knowing this.

It will definitely include more stuff beyond reading/writing configuration files, like the profiling utilities from #3, but the whole scope is a bit undefined at the moment. That's why I tried to keep the structure and API as simple as possible, so that they can be easily changed later on.

Are you planning on having the docs and API reference built and rendered somewhere? If yes, is there a tool that can handle your formatting? The tools I’ve used expect markdown/restructuredtext docs and specific formatting of docstrings (googledoc format added in this PR).

Yes, I do want to have the docs and API referenced built and rendered, but I'm still a bit unsure of the exact setup. That's why I didn't want to spent too much time properly formatting everything. But the sooner this gets decided and fixed the better, I guess.

I'm currently leaning towards a mkdocs+github pages setup, but I need to see how that renders. Any comments/suggestions about that would be appreciated.

Writing MOM_input files mucks up the spacing of the comments which makes them pretty difficult to read. I don't know whether this is worth fixing.

Yes, that's a bit unfortunate. This part is handled by the f90nml package, as I didn't want to reinvent the wheel (actually, I should probably also use it for the nuopc.runconfig files), so not quite under my control.

There’s some redundancy in the name of modules and their methods (e.g. read_mom6_input inside mom6_input). It may be nice to expose the read_* and write_* methods in the top-level __init__.py, so that instead of

from om3utils.mom6_input import read_mom6_input

I could do

from om3utils import read_mom6_input

But, as above, the best structure depends on what else might be added to the package.

That's a good point. Probably worth discussing more in detail when adding the profiling stuff, as that's quite orthogonal to the configuration files. It might become clearer then what's the best way to structure this.

[dougiesquire]

I'm currently leaning towards a mkdocs+github pages setup, but I need to see how that renders.

I've only used Sphinx and usually readthedocs and I like them. The ACCESS-NRI Land Team are using mkdocs for benchcab - you could ask Sean or Claire about how they've found it

[aekiss]

I have no experience with mkdocs or Sphinx, but MOM6 and CICE6 docs use Sphinx; MOM6 also documents code via Doxygen (both Sphinx and Doxygen have Fortran modes).
Sphinx is more powerful but mkdocs sounds easier to use from what I've read.

[micaeljtoliveira]

@aekiss @dougiesquire Thanks for the feedback! I've got some experience with both mkdocs and sphinx, but not for Python projects. I think I'll simply try both and see how it goes.

[aekiss]

ACCESS-Hive uses MkDocs https://access-hive.org.au/about/contribute/contribute_on_github/

[micaeljtoliveira]

@aekiss That's a good incentive to use mkdocs, as it's probably a good idea to make all the documentation as compatible as possible.