Towards resolving issue #14

[spencerkclark]

Here I make the changes I suggested in my comment in #14. In the process I realized that it might be important to make sure that the names of coordinates (within the DataArrays we pass around) should match the defined internal names within aospy; this currently is not done in either calc.py or model.py. For instance in the xray branch version of model.py a diff is done on coordinates named 'lonb' and 'latb'; these should instead be the internal names 'lon_bounds' and 'lat_bounds' respectively (not the names of the coordinates that were in the file that was read in). I now try to rename coordinates upon reading in the files so that this is no longer an issue.

Incidentally I also think we no longer should compute surface area within calc.py. See my comment in _add_grid_attributes within calc.py. I think the issue I was having should be resolved by me creating a new Model object, rather than having the existing Model try to accommodate the mis-aligned coordinates. That said, we should make things very clear via warnings or exceptions if coordinates in Runs and Models are indeed mis-aligned so the user can take the requisite steps to fix things.

The reason for my change of heart on this is that while it is fairly easy to re-compute surface area based on a new set of lat and lon coordinates, computing something like zsurf or land_mask on a slightly different grid is not as trivial. For that reason I feel like we'd never really satisfactorily be able to support slightly different grids (e.g. off in the 5th digit) for a single Model; it should really be a new model unto itself. While verbally I might say the model used for each of these cases was the same, for computational purposes in aospy the models are not.

That said, I still think it is valuable to do this (a) to check that the Model and Run grids are aligned and (b) for the convenience of having the peripheral grid information built into each DataArray.

[spencerahill]

Thanks a lot for all your hard work on this. I will take a closer look soon.

[spencerahill]

I put builtin imports first, followed by a space, followed by other package imports, followed by a space, followed by relative imports within the package. So this space should remain.

[spencerahill]

This looks good overall. I have made inline comments where there are some concerns/problems. I'd like to resolve those before making the merge. (Regarding the style-related ones, I realize that they may seem pedantic, but I'm a big proponent of keeping the style consistent, which includes, as much as possible, being PEP8 compliant.)

I agree that forcing names to be the same is the way to go -- that was the motivation behind creating the _STR constants at the package level. So we should use those throughout the code. More robust in case we needed to change them for some reason (not likely in this application, but just good practice to create variables for magic numbers/magic strings rather than typing in their value directly (which obviously I only recently have been adhering to myself)).

C.f. our most recent comments in #14 , would allowing the grid fields to be defined at the Run level resolve your issue with the slightly different lat/lon values? And would that change how we would handle the changes you've implemented.

Thanks a lot!

[spencerkclark]

No worries! I'll take care of the style issues and update the PR later today. Yes, allowing grid fields (in particular land_mask and zsurf) to be defined at the Run level would effectively close #14 I think.

[spencerahill]

Great, thanks. Also the commits that I have pushed since you made this (unrelated to this issue) made automatic merging not work, so if its not too much trouble please pull those first.

[spencerkclark]

I made the changes and it should be ready to merge automatically now -- I can't view the issues found by QuantifiedCode due to a 404 error on their end. What kinds of issues does it look for exactly?

[spencerkclark]

Actually hold up on merging just yet -- I think I have a cleaner solution than using the _strip_undefined_coords method. The reason I need to use that is that if a grid attribute is not found either in the Model or the Run then it adds a scalar coordinate with value None with the name of the attribute. This would cause problems for the idealized cases, (which don't have a 'level' attribute or 'land_mask' etc.), because these undefined scalar coordinates would get carried everywhere. When you tried to save (or compare to coordinates from the Model object) this would cause exceptions (or coordinate DataArrays that should be considered equal declared not equal due to the presence of the scalar coordinates).

I think a cleaner solution here is to just include some logic to _add_grid_attributes along the lines of "if the grid attribute is not in the Run or Model object then throw a Warning, but don't add it to the Dataset."

[spencerahill]

QuantifiedCode is a startup that has really cool code analysis and visualization tools. The site has been a little buggy though...I think they are still working out the kinks. I just invited you as an admin to aospy on their end, let me know if that doesn't solve the problem. In this case, one thing they are flagging is that you use isinstance(...) or isinstance(...), where the idiom would be to use a single isinstance with a tuple of types as the second argument. If this gets to be a nuisance, we can disable it. Also, FYI the Travis CI check is totally useless, becase I never set it up properly. Will do that someday.

(I heard about QuantifiedCode on this podcast, which I really like and you might too: http://talkpython.fm/ )

[spencerkclark]

Nice -- I think it was something on my end. I opened the link in another browser and things worked out fine. QuantifiedCode looks pretty cool. I went ahead and addressed all the issues it was finding with my PR (it helps me learn a few things in the process as well; I never really thought about static methods in python before now).

Great podcast recommendation as well. Thanks!

[spencerahill]

Awesome, thanks Spencer! This is great stuff.