BUG: fix handling of nan input for Time properties

[bsipocz]

This should fix the current test failure with the IRAS record from CADC.

Fix #460

[codecov]

Codecov Report

Merging #463 (533390f) into main (500ce4d) will decrease coverage by 0.08%.
The diff coverage is 11.11%.

@@            Coverage Diff             @@
##             main     #463      +/-   ##
==========================================
- Coverage   80.08%   80.00%   -0.08%     
==========================================
  Files          52       52              
  Lines        6020     6027       +7     
==========================================
+ Hits         4821     4822       +1     
- Misses       1199     1205       +6     

Files Changed	Coverage Δ
pyvo/dal/sia2.py	76.96% <11.11%> (-1.14%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[bsipocz]

(ignore the changelog failure, I was testing the milestone check and while it's been fixed, it doesn't trigger a rerun and thus a green checkmark)

[tomdonaldson]

Thanks @bsipocz . This is clearly better than before.

I do agree that it feels like there would be a better way to deal with this. It's unclear to me how (or currently if) this property is used, so maybe that would help, but I wonder how useful or important it is for this getter to return a Time object. Any opportunity for a getter to fail makes it more fragile.

With this one now returning either a time or something that isn't np.finite(), a consumer of the value likely still needs to check which it is, so they may as well be the ones to create the Time object if that's what they want.

[bsipocz]

The issue with returning a Time object is that there isn't really an empty time object, as opposed to e.g. a Quantity/etc.

That being said, I'm happy to return whatever you think is better. And I have just checked and SIA we return None for the dateobs property, so that's an easy thing to do here, too.

[tomdonaldson]

The issue with returning a Time object is that there isn't really an empty time object, as opposed to e.g. a Quantity/etc.

That being said, I'm happy to return whatever you think is better. And I have just checked and SIA we return None for the dateobs property, so that's an easy thing to do here, too.

Yes, maybe None is a little better in that it might be a more intuitive thing for a consumer to check on, though they still have to check, so it circles back to me not knowing how this is intended to be used. I'm happy either way for this PR.

[bsipocz]

so it circles back to me not knowing how this is intended to be used

honestly, I don't understand why pyvo is so heavy peppered through with these properties, IMO it that makes the API more complex, but it's likely I miss some point somewhere.

[msdemlei]

On Wed, Jul 26, 2023 at 10:57:09AM -0700, Adrian wrote: Wondering whether we should leak the `nan` here or just return `None`. @msdemlei does `nan` provide more meaning in this context?

VOTable, IMHO regrettably, does not distinguish None and NaN. In TABLEDATA and BINARY2 serialisation, we could (by and large) maintain this distinction, but the use of NaN as a NULL value is far too widespread to bother. So, nothing that's not already broken will break if you identify None and NaN for anything coming out of a VOTable.

[msdemlei]

On Wed, Jul 26, 2023 at 03:15:27PM -0700, Brigitta Sipőcz wrote: honestly, I don't understand why pyvo is so heavy peppered through with these properties, IMO it that makes the API more complex, but it's likely I miss some point somewhere.

Tangentially: I'm not a fan of the wide use of properties either. Not enough to break existing APIs, but I think for new APIs we should encourage less magic of this kind, even if it means that we have different API flavours in one package.

[bsipocz]

OK, so everyone seems to be happy with the None case, so I go ahead and merge this. Thanks for the fast review cycle!