Valid VOUnits fail check_units

[jwfraustro]

Sorry for having to open this issue, because I don't see a super-quick way to remedy it. :(

There are certain units that are valid VOUnits, but are not correctly supported by astropy, which means they will fail the @model_validator for a Column.

An example:

Column(
    name="log_mstar",
    id="dbo.targets.log_mstar",
    description="Logarithm of stellar mass",
    datatype="double",
    ivoa_unit="log(solMass)"
)
pydantic_core._pydantic_core.ValidationError: 1 validation error for Column
  Value error, Invalid unit: 'log(solMass)' did not parse as unit: 'log' is not a recognized function 

Failing here:

felis/python/felis/datamodel.py

Lines 264 to 272 in 71dbc85

	if fits_unit and ivoa_unit:
	raise ValueError("Column cannot have both FITS and IVOA units")
	unit = fits_unit or ivoa_unit
	if unit is not None:
	try:
	units.Unit(unit)
	except ValueError as e:
	raise ValueError(f"Invalid unit: {e}")

The syntax of log(solMass) is valid according to the VOUnit standard, however, astropy's units.Unit (or even units.format.VOUnit) doesn't support it. There are a large number of astropy issues (and more) about this.

I'm open to suggestions on how to tackle this. I usually hate options that let you bypass validation, but maybe it's possible?

[JeremyMcCormick]

We currently process units generically in validation like this:

units.Unit(unit)

Though the field in the YAML is ivoa:unit, which is somewhat misleading. There is a somewhat complicated backstory here where the IVOA units parser in astropy does not properly raise exceptions when it encounters invalid units. So that's why it isn't being used in Felis. We use the generic parser instead which is some mash-up of many different styles of units that attempts to be a superset of them.

In order to enable the IVOA unit parsing, I need to identify and test an astropy release which has a fix for this issue.

If you have any suggestions on interim solutions, please let me know.

[JeremyMcCormick]

After a little investigation, I found that the fix for raising errors with VO units seems to have made it into the astropy 7.0.0 release:

This command results in an exception being thrown, which is the desired behavior:

Unit("bad", format="vounit", parse_strict="raise")

ValueError: Unit 'bad' not supported by the VOUnit standard. Did you mean Ba (deprecated), aD, ad or g.cm**-1.s**-2?
If you cannot change the unit string then try specifying the 'parse_strict' argument.

So we should be able to switch over to using the VO unit parser. There is an existing ticket in the Jira on which the work will be done.

[JeremyMcCormick]

The plot thickens because Rubin is waiting on a fix for astropy/astropy#17528 before updating to 7.x.

But I'm thinking of implementing this with a check on astropy version which would use the old behavior as a fallback. Then you could use astropy 7.x locally with IVOA unit parsing while Rubin uses 6.x for the time being until this fix is incorporated into an astropy patch release.

[JeremyMcCormick]

@jwfraustro It seems like astropy's VO Unit parse doesn't like your unit string:

File ~/miniconda3/envs/felis_env/lib/python3.12/site-packages/astropy/units/core.py:2144, in _UnitMetaClass.__call__(self, s, represents, format, namespace, doc, parse_strict)
   2134     msg = (
   2135         f"'{s}' did not parse as {format_clause}unit: {str(e)} "
   2136         "If this is meant to be a custom unit, "
   (...)
   2141         "https://docs.astropy.org/en/latest/units/combining_and_defining.html"
   2142     )
   2143     if parse_strict == "raise":
-> 2144         raise ValueError(msg)
   2145     warnings.warn(msg, UnitsWarning)
   2146 return UnrecognizedUnit(s)

ValueError: 'log(solMass)' did not parse as vounit unit: 'log' is not a recognized function If this is meant to be a custom unit, define it with 'u.def_unit'. To have it recognized inside a file reader or other code, enable it with 'u.add_enabled_units'. For details, see https://docs.astropy.org/en/latest/units/combining_and_defining.html

So I guess my suggested remedy is not going to fix this...