NOAA-ORR-ERD / gridded

A single API for accessing / working with gridded model results on multiple grid types
https://noaa-orr-erd.github.io/gridded/index.html
The Unlicense
64 stars 14 forks source link

fixed the __eq__ check,and acouple bugs about out of bounds. #83

Open ChrisBarker-NOAA opened 8 months ago

ChrisBarker-NOAA commented 8 months ago

added lots of tests -- up to 96% coverage.

tiny bit of refactoring.

@jay-hennen: I'm not merging right away because I'm not sure about the interp_alpha function for a constant_time Time series (i.e. one with only one datetime). It is always returning 0.0 -- shouldn't it always return 1.0? or maybe 0.0 for before, and 1.0 for after??

I'm got a known fail test in test_time for this -- could you please decide and document what it's supposed to do, and make the test correct.

It could be that there's never a reason to call interp_alpha for constant time data -- but them what is constant_time for? In any case, if it's useless, it should raise an error maybe.

Anyway, after addressing this one issue, please merge -- this is not a good bug.

Here's a few more thoughts in the Time object that we can think about later: https://github.com/NOAA-ORR-ERD/gridded/issues/82

Also: The eq for grids seems to be VERY broken, that could cause issues! And we should probably have a eq for Variables -- I think in PyGNOME, it's using the one provided by Gnome_id, but seems it should be built in.

https://github.com/NOAA-ORR-ERD/gridded/issues/81

jay-hennen commented 8 months ago

Any change to interp_alpha behavior should be done in conjunction with index_of and Variable._time_interp. I'm happy with the way its been the last seven years or so but if you want to change it or fix it go ahead.

interp_alpha is a totally valid call on a constant Time object. Variable._time_interp would probably explain the values it gives the best.

gridded has never really had a need for eq. In GNOME object comparison is usually a big deal because of serialization, but that doesn't really apply to Grids or Variables because nothing of importance is saved as JSON. I suppose it does matter for Time because that does get saved in the JSON.

ChrisBarker-NOAA commented 8 months ago

Any change to interp_alpha behavior should be done in conjunction with index_of and Variable._time_interp. I'm happy with the way its been the last seven years or so but if you want to change it or fix it go ahead.

OK, I'll look then, I thought you might remember what's going on. But the behaviour should be well defined and documented.

interp_alpha is a totally valid call on a constant Time object.

Which makes sense -- but it's always returning 0.0 -- which doesn't make sense -- I can't see how that could possibly be useful.

Variable._time_interp would probably explain the values it gives the best.

I'll take a look there -- this is a bit of an intertwined API, s a bit hard to know where behavior is defined.

gridded has never really had a need for eq. In GNOME object comparison is usually a big deal because of serialization, but that doesn't really apply to Grids or Variables because nothing of importance is saved as JSON.

Gridded isn't a sub-module of PyGNOME -- it's it's own thing. But anyway, a broken __eq__ is potentially dangerous. If you don't define it, then, I think, comparing will only return equal if it's the same object -- which is fine -- I" remove the Grid __eq__

I suppose it does matter for Time because that does get saved in the JSON.

Well, it's getting used in the code that's trying to manage references -- but maybe object identity is all we need there -- the trick is that it's using the build in in -- which checks equality as well.

The point is that ANY class in Python should have a well-behaved eq -- e.g. it shouldn't raise, and it shouldn't return True when the objects are not truly equal.

ChrisBarker-NOAA commented 8 months ago

merging -- not a lot changed, but one important bug -- and some added tests and lint cleanup