-
Notifications
You must be signed in to change notification settings - Fork 19
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
Added Cheat Sheet #656
Added Cheat Sheet #656
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.
Looks great. Some suggestions ...
Co-authored-by: David Hassell <[email protected]>
I'm aware this is still in draft form, but from a quick peek it is looking fantastic. If you'd like a second review (though probably not necessary) feel free to assign me when it is ready. |
Thanks for the suggestions David! |
Hi Sadie, I have marked it ready for review! You could either generate the documentation on your system or just head to https://github.com/bewithankit/cf-python/blob/cheatsheet/docs/source/cheat_sheet.rst where it renders properly! |
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.
I've raised several minor comments, but overall this is looking excellent and will be very useful to users, I am sure. Let me know what you think, but this close to being merge-ready IMO...
Co-authored-by: Sadie L. Bartholomew <[email protected]>
Thanks for addressing the feedback, Ankit. @davidhassell are you happy with the present state, in which case I think we can merge? |
Hi Sadie, thanks again for the suggestions. As the Cheat Sheet will be an addition to the sidebar menu, all the pages will need to be regenerated again. I think David said we'll add it in the next release as it'll be easier that way. |
Sounds good. I'm looking forward to seeing it in the documentation. Thanks Ankit. |
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.
Hi Ankit - really liking this. A few comments ...
| | >>> jan_2023 = temp.subspace(T=cf.year(2023) & cf.month(1)) | | ||
| | >>> annual_avg_61_90 = annual_global_avg.subspace(T=cf.year(cf.wi(1961, 1990))) | | ||
+----------------------------------------------------------------------------------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | ||
| **Accessing constructs** | *Select constructs by identity:* | |
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.
I think that the emphasis in this section should be on the named-by-construct methods (dimension_coordinate
, auxiliary_coordinates
, etc.) rather than the generic ones (construct
, constructs
). An awareness of the generic ones is good, but they're not really what people use, I think. E.g. I would hardly ever use f.constructs.filter_by_type('dimension_coordinate')
instead of f.dimension_coordinates()
; but I might well use f.constructs.filter_by_type('dimension_coordinate', 'field_ancillary')
.
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.
Thanks for the suggestion David. I thought users might not know all the metadata constructs so thought of putting the constructs
container! I have done the changes, let me know if those are correct, thanks!
docs/source/cheat_sheet.rst
Outdated
| | >>> d = f.constructs.filter_by_ncvar('time') | | ||
| | >>> d = f.constructs.filter_by_ncvar('time', 'lat') | | ||
+----------------------------------------------------------------------------------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | ||
| **Regridding** | **Spherical regridding for regridding data between domains with spherical coordinate systems:** | |
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.
| **Regridding** | **Spherical regridding for regridding data between domains with spherical coordinate systems:** | | |
| **Regridding** | **Spherical regridding for regridding data between domains with spherical coordinate systems:** | |
This title (and the description in the tutorial) perhaps could be tweaked to make it clear that you can go to/from/between plane projections of spherical coordinate systems too. Can't think of anything pithy yet, though ....
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.
Having just thought about the Cartesian case, what about
Regridding in spherical polar coordinates
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.
Hi David, I didn't get this! Can you explain?
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.
Hi ANkit, sorry, it did get a bit confusing! I'm suggesting replacing **Spherical regridding for regridding data between domains with spherical coordinate systems:**
with
**Regridding in spherical polar coordinates**
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.
Great. One minor suggestion - merge at will.
Co-authored-by: David Hassell <[email protected]>
Hi David, just wanted to confirm if I should merge it now or do it in the next release as we had discussed? |
Hi - I think that you can merge it now, it just won't show up till the next release when we rebuild the docs (well the it will appear in the side bar of index.html, but won't work - that's OK for a few days) |
Resolves #591.