Skip to content
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

Remove obsolete Constant and Units classes #50

Closed
spencerahill opened this issue Dec 29, 2015 · 13 comments
Closed

Remove obsolete Constant and Units classes #50

spencerahill opened this issue Dec 29, 2015 · 13 comments

Comments

@spencerahill
Copy link
Owner

In particular, multiplying a Constant object with an xray object results in a numpy array, rather than an xray object as desired. This is just one example of multiple problems with the operator overloads for the Constant class...I didn't really know what I was doing when I wrote them! So all of them need to be inspected, likely re-written, and tested.

As a workaround until then, one can just use the value attribute, e.g. c_p.value rather than just c_p.

@spencerahill
Copy link
Owner Author

These bugs are also causing failing tests.

@spencerkclark
Copy link
Collaborator

I noticed some remaining messy logic related to constants as I was reviewing #178. E.g. in calc.py:

 # Pass numerical constants as is.
 if isinstance(var, (float, int)):
     return var
 elif isinstance(var, Constant):
     return var.value

Returning just the value associated with the constant is needed because the operators (as stated in issue) don't work as desired.

It occurred to me that what's implemented in constants.py was basically an attempt to create numbers (which you can do mathematical operations with arrays with) with attached metadata (implemented pre-xarray's introduction to aospy). I think the basic functionality desired here could be obtained by simply using single-valued DataArrays with 'units' and 'description' attributes. Then we would get numerical operator support for free. E.g.

R_d = Constant(
    287.04,
    'J/K/kg',
    description='Dry air gas constant'
)

could instead just be:

import xarray as xr

R_d = xr.DataArray(287.04, attrs={'units': 'J/K/kg', 'description': 'Dry air gas constant'})

Not a huge priority, but just wanted to get these thoughts down while they were fresh in my head. Is there any reason to have a separate class for Constant values, or could this DataArray solution suffice?

@spencerahill
Copy link
Owner Author

basically an attempt to create numbers (which you can do mathematical operations with arrays with) with attached metadata

Exactly

I think the basic functionality desired here could be obtained by simply using single-valued DataArrays with 'units' and 'description' attributes.

Very good point. I don't see any reason now or in the future why we the DataArray version wouldn't suffice. So I'm in favor of dropping the Constants class entirely and replacing the ones we have defined with DataArrays.

While we're at it, I'm not sure that the Units class is really in the scope of the project. I mainly used it for plotting, but we're no longer handling plotting anyways. What do you think?

@spencerkclark
Copy link
Collaborator

So I'm in favor of dropping the Constants class entirely and replacing the ones we have defined with DataArrays.

Sounds good -- I'll get around to putting together a PR at some point to clean that up.

While we're at it, I'm not sure that the Units class is really in the scope of the project. I mainly used it for plotting, but we're no longer handling plotting anyways. What do you think?

I totally agree. It may still make sense to add string-based units attributes to Var objects, and propagate those out to saved netCDF files, but I see no issue removing the Units class. In the future we could consider leveraging a more full-fledged units library to handle some things, but it's probably best to wait and see what happens elsewhere (I know there's been discussion on and off in xarray about it).

@spencerahill
Copy link
Owner Author

I'll get around to putting together a PR at some point to clean that up.

Thanks! We can do Units and Constants together. We'll also need to remove them from the API ref section of our docs, where they're briefly described.

It may still make sense to add string-based units attributes to Var objects, and propagate those out to saved netCDF files, but I see no issue removing the Units class. In the future we could consider leveraging a more full-fledged units library to handle some things, but it's probably best to wait and see what happens elsewhere (I know there's been discussion on and off in xarray about it).

My thoughts exactly. I created the Units class originally with the intention of making all calculations units-aware, which would still be awesome, but units-aware arrays are definitely not something we want to try to roll ourselves. Will see how the xarray-side things develop.

@spencerahill spencerahill changed the title Constant class operator overloads don't work as intended Remove obsolete Constant and Units classes May 4, 2017
@spencerahill spencerahill modified the milestones: before v1.0, v0.2 Aug 31, 2017
@spencerahill
Copy link
Owner Author

@spencerkclark @chuaxr in talking with @micahkim23 about #201, it occurs to us that Var automatically casts its units attribute to be a Units object. Given that we want to deprecate Units, this seems bad.

So if we do go ahead with deprecating Units, it will require some modification of your object libraries, in particular if you have defined your Var objects like I have by passing Units objects to the Var.__init__ units kwarg. So I just wanted to get your take on that. I'm happy to update my library. Hope that makes sense.

@spencerahill
Copy link
Owner Author

spencerahill commented Oct 18, 2017

To clarify: if anybody has defined aospy.Units objects in their object library and passes them directly to their aospy.Var objects, e.g. myvar = Var(..., units=myunitsobj, ...), then once we have deprecated aospy.Units, you will have to change those to be myvar = Var(...,units='some-string-that-specifies-the-units', ...), where (unless you want to change the string representation of those units) that string would be identical to myunitsobj.units.

@chuaxr
Copy link

chuaxr commented Oct 18, 2017

@spencerahill I haven't created any Units objects, so I have no objections :)

@spencerkclark
Copy link
Collaborator

@spencerahill I'm also happy to update my object library. Go forward with the deprecation!

@spencerahill
Copy link
Owner Author

spencerahill commented Oct 20, 2017

Great, thanks @spencerkclark and @chuaxr .

@micahkim23, open a PR whenever you're ready. Be sure to grep for Units throughout the entire codebase to find everywhere the Units class is referenced. I.e. from the top-level aospy directory grep -n Units -r . Or since you're using PyCharm I suspect it has a tool for doing this. That includes our documentation.

Ideally you'll issue the PR once you feel it's fully ready or nearly so. But if something hangs you up along the way, it's a good idea to submit a "work-in-progress" (WIP) PR in order to elicit feedback on the problem.

@spencerahill
Copy link
Owner Author

This Issue relates to the obsolete Constants class as well. But I think it's prudent for @micahkim23 to just deal with Units for now, so that we can tackle the more pressing #201, and then eventually come back to Constants.

Once we do get there, the same disclaimer I mentioned above for Units applies for Constants: anybody that's using them in their object library will have to remove them. The most kosher way of doing this would be a formal deprecation cycle, but I think that would be overkill at this stage.

Nevertheless, it will be appropriate given this breaking change to bump to v0.3 rather than v0.2.1 as the next release.

@micahkim23
Copy link
Collaborator

@spencerahill alright, I will focus on Units for now.

@spencerahill
Copy link
Owner Author

#222 removed Units, so now we're just left with Constants.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants