-
Notifications
You must be signed in to change notification settings - Fork 12
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
WIP got rid of Units class #222
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.
@micahkim23 this looks great! Thanks for putting this together. I just have one comment within the diff for the documentation.
It would also be great if you could add a note in whats-new.rst
regarding this change. As @spencerahill suggests (#50 (comment)), please update the version number for the "unreleased" heading to 0.3
. I think this would probably be best noted under a new "Breaking changes" heading (since this will require some users to modify their object libraries).
docs/api.rst
Outdated
@@ -122,6 +122,12 @@ Var | |||
|
|||
.. automethod:: aospy.var.Var.__init__ | |||
|
|||
.. note:: | |||
|
|||
There has been discussion of implementing units-handling upstream |
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 realize you're just copying this text from the old documentation for Units
objects (which is totally fine), but I think it might be good to take the opportunity to be a little more descriptive here. Maybe something along the lines of:
.. note::
While for the sake of tracking metadata we encourage users to add a ``units``
attribute to each of their :py:class:`Var` objects, these ``units`` attributes provide
nothing more than descriptive value. One day we hope DataArrays
produced by loading or computing variables will be truly units-aware
(e.g. adding or subtracting two DataArrays with different units will lead to
an error, or multiplying two DataArrays will result in a DataArray with new units),
but we will leave that to upstream libraries to implement
(see `pydata/xarray#525 <https://github.com/pydata/xarray/issues/525>`_
for more discussion).
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.
@spencerkclark thanks for the suggestion, I'll fix that.
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 agree, and just add 'the' between 'for' and 'sake'.
Thanks @micahkim23. Looks good overall. I agree w/ @spencerkclark 's comments. Once those are addressed this should be good to go! |
I was just wondering, since this change is supposed to be part of |
There's actually nothing that is formally binding us to v0.2.1 at this point. Just change the |
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 looks good to me; just a couple minor nits!
docs/whats-new.rst
Outdated
~~~~~~~~~~~~~~~~ | ||
|
||
- Deprecate ``Units`` class, so now the ``units`` attribute of the | ||
``Var`` class is a ``string``. (fixes :issue:`50` via pull:`222`). |
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.
- Remove the double backticks from "string" (in other words make it ordinary text rather than a literal, because the literal name of the datatype is
str
) - Add a colon before "pull"
In it goes! Thanks @micahkim23 for the heavy lifting and @spencerkclark for the review. |
@spencerahill How does this look?