spencerahill / aospy

Python package for automated analysis and management of gridded climate data
Apache License 2.0
83 stars 12 forks source link

Refactor pressure handling in calc.py #287

Closed spencerkclark closed 6 years ago

spencerkclark commented 6 years ago

@spencerahill in the spirit of cleaning up Calc, I figured I would take a shot at refactoring the pressure logic. This is not completely addressing #80; however, I do feel it is an improvement from what we currently do.

Here I make pressure variables first-class Var objects and load data for them just as we would for any other variable. This required a couple of changes:

I think I should add some explicit tests for passing a Model object to data_loader.load_variable, but this is done implicitly in the existing test suite, and things are passing. This would also be a good time to document how to use pressure variables in one's own computed Var objects.

Before I do that though, do you agree with this approach?

spencerkclark commented 6 years ago

But it feels a bit out of place there, like we're shimming it in. What do you think?

Yes, it did feel a shame to make load_variable more complicated. I went with your suggestion in my latest commit: 9829549.

spencerahill commented 6 years ago

I went with your suggestion in my latest commit: 9829549.

Thanks. Initially, I was envisioning moving it out of DataLoader entirely, but now I see that how you've done it is better...it enables DataLoaders of variables that rely on grid attrs to be entirely self-contained, rather than relying on Calc to grab the grid attrs for them if they can't be found on disk otherwise.

spencerkclark commented 6 years ago

Indeed my primary motivation for doing that was to be able to support recursive calculations where a Var object standing in for a grid attribute was required for computing a variable that was not in the top level of recursion.

spencerkclark commented 6 years ago

@spencerahill when you get a chance I think this should be ready for a final review.

spencerkclark commented 6 years ago

Thanks for your comments @spencerahill! Merging now.

spencerahill commented 6 years ago

Thanks @spencerkclark! Along with my smaller ones preceding this, we've really started to make a dent in cleaning up calc.py. And all the more so once we get the Reductions in.