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

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

Open
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

ChrisBarker-NOAA
Copy link
Contributor

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:
#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.

#81

@jay-hennen
Copy link
Contributor

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
Copy link
Contributor Author

ChrisBarker-NOAA commented Nov 29, 2023

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
Copy link
Contributor Author

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

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

Successfully merging this pull request may close these issues.

2 participants