BUG: Fix VOTable unit format on write so it does not hardcode to CDS format always#17570
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
|
👋 Thank you for your draft pull request! Do you know that you can use |
| if "unit" in attrib: | ||
| attrib["unit"] = self.unit.to_string("cds") | ||
| format = _get_unit_format(self._config) | ||
| attrib["unit"] = self.unit.to_string(format) |
There was a problem hiding this comment.
Thank you for writing the PR!
An other issue pointed by Tom in the original thread was that the standard does not impose a format for the units, but only recommends it. So using any format (even out of cds and vounit) should raise (at most) a warning and not an error.
Maybe the call of unit.to_string should catch any ValueError and transform it into a warning?
There was a problem hiding this comment.
We could do that but I fear the side effects. Would be nice if we have a realistic use case of such violation that should be allowed to add to the test suite. Since I am not VO expert, do you have any suggestion on such a test? I would like to use it to make sure both read/write work. Thanks!
There was a problem hiding this comment.
@ManonMarchand , are you able to assist in providing a test case to push this forward? Thanks!
There was a problem hiding this comment.
Yes, sorry.
Could be an exact reproducer of #17497 :
from astropy.table import Table
import astropy.units as u
import tempfile
table = Table({'a': [1, 2, 3]})
table["a"].unit = u.Unit("mJy / beam")
fp = tempfile.TemporaryFile()
table.write(fp, format="votable")This raises an error while it should be a warning (at most).
There was a problem hiding this comment.
@ManonMarchand , what I am looking for is a use case where a blind catch of ValueError is actually necessary and essential for VO operations. The example you provided in #17570 (comment) currently throws ValueError on main without this patch, but works fine with this patch already.
p.s. Instead of tempfile.TemporaryFile(), I used io.BytesIO() but that shouldn't matter.
There was a problem hiding this comment.
Good that it works now. Then, since the standard only recommends to use standard units but does not enforce it, things like deprecated units
table["a"].unit = u.Unit("Crab", format="ogip")or even invented ones
table["a"].unit = u.def_unit("my_unit")Should pass too. This time with warnings for sure.
I think this would translate Tom's comment here #17497 (comment)
We also need to make sure that these can be read too
There was a problem hiding this comment.
Reading #17497 (comment) again, I am a little worried that it goes against the philosophy of "lenient on read, strict on write." Turning exception to warning would encourage "bad behaviors" of continuing to write out non-standard information ("they are not stopping us, why fix?"). That said, I was not in those VO discussions, so I will see what I can do to this PR to honor the request. Thanks for the examples!
There was a problem hiding this comment.
I wasn't there neither. @tomdonaldson : what are your thoughts?
Since @msdemlei is one of the authors of the standard, maybe you have a clearer view on what should be done in terms of writing non-VOunits in VOTables? Warn or prevent it?
|
On Thu, Feb 06, 2025 at 08:14:33AM -0800, Manon Marchand wrote:
Since @msdemlei is one of the authors of the standard, maybe you
have a clearer view on what should be done in terms of writing
non-VOunits in VOTables? Warn or prevent it?
I first thought this was a good place for the golden rule of
interoperability: Be lenient in what you accept (i.e., even from a
VOTable, try to make sense of whatever is there) but be strict in
what you produce (i.e., don't silently dump invalid units into files
you write).
In this case, that's tricky, though, because I would expect that a
large fraction of tables we read are tables we will have to write a
bit later, if modified to some extent.
So, as Tom said, erroring out on invalid units is just not an option
we have, either on reading or writing.
Warning on reading bad units for normal Tables would, I think, be too
noisy; this is probably different for QTables, but I have too little
experience with those.
Slowly migrating people towards standards-friendly units would be
great, though. If there is an option to try and serialise parsed
units into VOUnits on VOTable writes would be great. Would we issue
a warning if that mutates the unit string? That depends how much we
trust the unit machinery; if we are confident that we always preseve
the semantics, we should, I'd say, just silently update the syntax.
If a unit couldn't be parsed on input or is not expressable in VOUnit
("dex"), I'd say we should just write whatever we read and emit a
warning -- ideally with a link to a soap box page that tells people
how to fix common cases (e.g., blanks, ^ vs. **, dex, mSol, etc).
|
|
Thanks Markus! |
|
@msdemlei , thank you for your detailed explanation. I am afraid that auto fixing invalid input units so they are compliant on read would be beyond my VO skills. Is this PR okay as is? If not, we can also close without merge and let someone expert in VO take over. Please advise. Thanks!
I need to figure out how test should handle warning in double run. Does not affect actual proposed functional code. |
|
On Thu, Feb 06, 2025 at 09:34:03AM -0800, P. L. Lim wrote:
@msdemlei , thank you for your detailed explanation. I am afraid
that auto fixing invalid input units so they are compliant on read
would be beyond my VO skills. Is this PR okay as is? If not, we can
Sure, I think this is clear progress, and for all I can see this does
what I was asking for: Write the unit in VOUnit if you can, and warn
and do a best effort if not.
Let's merge, as far as I am concerned (disclaimer: I *still* have not
sufficiently dug into astropy's VOTable code, and so I cannot drop
the "as far as I am concerned").
|
|
On the testing side, the double run is giving me pain. I tried several ways but nothing would make the warnings behave consistently on the second run for this new test. I might need to investigate in a separate PR for the double run job itself before I can get back to this. |
0db9094 to
b4104fb
Compare
| assert stored_votable["sfr"].unit == u.solMass / u.year | ||
|
|
||
|
|
||
| def test_write_jybeam_unit(tmp_path, recwarn): |
There was a problem hiding this comment.
Please do review carefully whether the tested behavior makes sense to you.
Might wanna ignore the double run rabbit hole. I tried tweaking
astropy/astropy/io/votable/exceptions.py
Line 66 in 07df8c9
or calling
astropy/astropy/io/votable/table.py
Line 409 in 07df8c9
but that just broke even more tests...
There was a problem hiding this comment.
Update: double run should be handled now via recwarn. Please do still review carefully.
|
On Fri, Feb 07, 2025 at 05:45:07PM -0800, P. L. Lim wrote:
@ManonMarchand @msdemlei -- Do you have opinion whether this should
be `UnitsWarning` or
[W50](https://docs.astropy.org/en/latest/io/votable/api_exceptions.html#w50-invalid-unit-string-x)?
Uh -- I can't really advise there I'm afraid, as I don't know enough
about who is using this when. My uninformed gut feeling would be:
W50 should probably be derived from UnitsWarning, too, with the
rationale that people who have silenced UnitsWarning-s probably won't
want to see W50 either; and people who have silenced VOTable warnings
won't want to see it either, so we'd have multiple inheritance and
need to hope that the warnings filter actually works the way I hope
it does.
But discard my voice if you want: As I said, this is not more than a
gut feeling.
|
While that makes sense, it is out of scope here and probably a very breaking change. The decision in this PR solely is about whether to emit a W50 or UnitsWarning, without changing behavior of either. I can also try ask the pyvo channel on Astropy Slack and see. Thanks! |
|
On Mon, Feb 10, 2025 at 08:29:29AM -0800, P. L. Lim wrote:
The decision in this PR solely is about whether to emit a W50 or
UnitsWarning, without changing behavior of either.
In that case my vote is now firmly in favour of W50, because I reckon
it's far more likely that people who don't want to see the warning
would have silenced those than the UnitsWarning; my reasoning is that
they will typically be more in a VOTable-writing mode than it a units
processing mode.
|
on write so it does not hardcode to CDS format always.
Co-authored-by: Eero Vaher <eero.vaher@gmail.com>
Update test and change log to match.
with votable warning machinery
|
I changed the votable warning to W50 and also rebased. FYI. |
msdemlei
left a comment
There was a problem hiding this comment.
I am probably missing the intricacies of the test_jbeam_unit test, so all I can say is that it looks plausible. Other than that, I think this is the behaviour we want, so: Let's go.
|
Thanks, @msdemlei ! @ManonMarchand , does your approval from last week still stand? |
|
It does, thanks again for fixing this! |
…t does not hardcode to CDS format always
|
Thanks for your reviews! Please let us know if this is still insufficient. This patch would land in nightly wheel tomorrow but if you need it sooner, please also let me know. |
…570-on-v7.0.x Backport PR #17570 on branch v7.0.x (BUG: Fix VOTable unit format on write so it does not hardcode to CDS format always)
* Add policy for adding new role * Address astropy/astropy#17570 (review) * Address dhomeier comments
Description
This pull request is to fix #17497 . Also see #11032 (v4.3)
Blocked by