Check for nan dateobs before converting to Time #550
Conversation
|
Just saw @ManonMarchand tagged this to the 1.5.2 bugfix milestone. Didn't see an entry for this version, so I moved my changelog entry to a new 1.5.2 header. Feel free to edit if I didn't quite get it right! |
|
Awesome, thanks! Would you be comfortable adding a test? A suggestion on how to proceed could be :
And it would also be nice to rebase the commits on the changlelog to group them together. If you need help with this don't hesitate to ask. |
Check for nan dateobs before converting to Time
Check for nan dateobs before converting to Time
| try: | ||
| if not dateobs or np.isnan(dateobs): | ||
| return None | ||
| except TypeError: | ||
| # np.isnan can only check floats. If can't check for nan, pass it along | ||
| pass |
There was a problem hiding this comment.
Looks like @bsipocz beat me to the punch before I could say anything! Thanks for the merge!
Some small context behind the try except on why I moved the check nan check to this try except, When testing I ran into this same error on stackoverflow: https://stackoverflow.com/questions/52657223/typeerror-ufunc-isnan-not-supported-for-the-input-types-and-the-inputs-could
Since the suggestion was to use pandas which isn't a current dependency for pyvo, I figured it was too much weight just for this small test to add a whole new dep. So the intention was to have np.isnan check the conditions it is able to, but to allow anything outside of those bounds to flow through to Time as before.
There was a problem hiding this comment.
This all looked good, the comment in the code seemed sufficient, and you addressed the review requests, too, so no more explanation is needed.
And indeed, we would have asked you to write a workaround if you introduced pandas as a dependency here.
This small PR fixes #549 by adding an additional check on the ucd value of
VOX:Image_MJDateObsbefore trying to construct anastropy.Timeobject. Bug found in SIA, though this PR ALSO preemptively performs the same check for SSA'sssa:DataID.Date.Changelog will be added immediately after this PR is published, and a PR number is assigned