-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve docs tutorial #164
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spencerahill this looks very good! Thanks for all your effort here cleaning up the internal links in the docs, fleshing out the submit_mult_calcs
docstring, and greatly improving the examples page by adding more description of the aospy workflow, and porting it to mirror (and ultimately use) the example_obj_lib
. Adding the scientific details as footnotes is also a really nice touch.
I've gone through your changes so far and offered some comments/suggestions; hopefully you find them helpful.
aospy/automate.py
Outdated
- None : no vertical reduction | ||
- 'vert_av' : mass-weighted vertical average | ||
- 'vert_int' : mass-weighted vertical integral | ||
input_time_intervals : {'annual', 'monthly', 'daily', '6hr', '3hr'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe put an "e.g." here to note that these are not the only options (and that the values one needs to specify here are controlled by how one sets up their DataLoaders).
aospy/automate.py
Outdated
calculations to be performed and prompt user to confirm before | ||
submitting for execution. | ||
- parallelize : (default False) If True, submit calculations in | ||
parallel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we note here that the way this is set up currently, this requires installing an optional dependency (pip install multiprocess
)?
aospy/automate.py
Outdated
- parallelize : (default False) If True, submit calculations in | ||
parallel. | ||
- write_to_tar : (default True) If True, write results of calculations | ||
to .tar files, one for each object. These tar files have an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"one for each object" feels somewhat ambiguous here; I think you mean "one for each :py:classaospy.Run
"?
aospy/automate.py
Outdated
parallel. | ||
- write_to_tar : (default True) If True, write results of calculations | ||
to .tar files, one for each object. These tar files have an | ||
identical directory structures the standard output relative to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is possibly a bug, but I'll just note that this is not entirely true at the moment. For example the example main script, aospy/examples/aospy_main.py
, produces the following directory structures:
Normal output:
example-output/example_proj/example_model/example_run
Tar output (note the data
directory under example_proj
):
example-tar-output/example_proj/data/example_model/example_run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I agree, this is a bug in my view. I will fix it within this PR if you agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go for it!
docs/using-aospy.rst
Outdated
@@ -239,7 +233,8 @@ and in the directory structure within which they are saved. | |||
- File name : | |||
``varname.intvl_out.dtype_out_time.'from_'intvl_in'_'dtype_in_time.model.run.date_range.nc`` | |||
|
|||
See the API reference documentation of ``CalcInterface`` for explanation of each of these components of the path and file name. | |||
See the API reference documentation of ``CalcInterface`` for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should you use :py:class:aospy.CalcInterface
here?
docs/examples.rst
Outdated
'../aospy/test/data/netcdf/00040101.precip_monthly.nc', | ||
'../aospy/test/data/netcdf/im.landmask.nc' | ||
rootdir + '/aospy/test/data/netcdf/00040101.precip_monthly.nc', | ||
rootdir + '/aospy/test/data/netcdf/im.landmask.nc' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be confusing to users why we are specifying a land mask for an aquaplanet simulation; should we make a note about this requirement? In this case it should1 be all zeros at all latitudes and longitudes.
1Good thing I checked...it turns out that it isn't. This is a land mask for a rectangular continent. I will submit a PR to correct this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #165 for the fix
docs/examples.rst
Outdated
calcs[0].path_out | ||
calcs[0].data_out | ||
|
||
**(S. Hill: Still need to finish up everything below this point)** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I recognize you're not done here, but I'm adding some comments anyway :)
docs/examples.rst
Outdated
|
||
You may have noticed that ``subset_`` and ``raw_`` coordinates have | ||
years 1678 and later, when our data was from model years 4 | ||
through 6. This is because technical details upstream (in numpy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This issue is complicated and there are multiple levels of upstream dependencies; instead of calling out numpy in particular, I might just link to the discussion in this issue: pandas-dev/pandas#7307
docs/examples.rst
Outdated
You may have noticed that ``subset_`` and ``raw_`` coordinates have | ||
years 1678 and later, when our data was from model years 4 | ||
through 6. This is because technical details upstream (in numpy) | ||
limit the range of supported years to roughly 1677 to 2234. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's link directly to this part of the pandas docs here: http://pandas.pydata.org/pandas-docs/stable/timeseries.html#timestamp-limitations
Also the upper bound year should be 2262 (instead of 2234).
docs/examples.rst
Outdated
We can re-use our object library at will to perform new calculations | ||
or re-compute old ones. We can also add new objects. For example, | ||
suppose we performed a new simulation in which we increased the | ||
surface albedo or introduced a rectangular continent. All we would |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case of a rectangular continent, I might say "not so fast!" :)
Currently the safest thing to do in that situation would be to create a new Model with a different land mask (to make sure regional calculations that depended on land would be accurate). Changing surface albedo is fine for the same Model, however.
If you want some other parameters to tweak as an example (while keeping the same Model object) you could say increase the solar constant or turn off the convection scheme.
@spencerkclark thanks a lot! Won't bother responding in-line, because it all sounds good. Will merge #165 also. |
@spencerkclark ok I've addressed all your comments and finished the Examples section. I will go through all of the docs pages later today one more time...I suspect there are some out of date parts in Using aospy especially, given all the recent modifications. After that I'd say ready to merge. |
This looks great to me; go ahead and merge! Thanks again.
Sounds good; I'll take care of updating the example Jupyter notebook. I think I should be able to get our CI configured to test that as well. |
Ok, finished going through all the docs again. Merging pending CI pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just caught a minor typo.
docs/using-aospy.rst
Outdated
|
||
:py:func:`aospy.submit_mult_calcs` creates a :py:class:`aospy.CalcSuite` | ||
object that permutes over the provided lists of calculation | ||
specifications, encoding each permutations into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: "each permutations"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, thanks
Closes #151
@spencerkclark with apologies for poor git practices, I ran out of steam before I could finish the last chunk of the Examples page (I noted the point where I stopped).
Will return to this tomorrow. I feel like it's mostly there, but of course any suggestions welcome.