Quantity.is_compatible (src/modelskill/quantity.py:61) is meant to let an undefined quantity compare cleanly against anything. It does not, because it tests for the wrong sentinel.
from modelskill import Quantity
u = Quantity.undefined()
wl = Quantity(name="Water Level", unit="meter")
u # Quantity(name='', unit='')
wl.is_compatible(u) # False <- expected True
u.is_compatible(wl) # False <- expected True
wl.is_compatible(Quantity("Undefined", "Undefined")) # True
Two separate problems:
1. Wrong undefined sentinel. The method checks self.name == "Undefined", but Quantity.undefined() returns Quantity(name="", unit=""). Only the literal string "Undefined" is recognised, which is not what the constructor produces.
2. Unit-strict. Beyond the undefined case it falls back to self == other, so two quantities with the same name but different units never agree:
Quantity("Pressure", "").is_compatible(Quantity("Pressure", "MetresWater")) # False
This case is not hypothetical. Model results read from res1d and EPANET files carry a quantity name but no unit — mikeio1d exposes quantities as a plain list of names — so a network model result is legitimately Pressure [] while the matching observation is Pressure [MetresWater].
Why it has gone unnoticed
is_compatible is called from nowhere in src/ or tests/. Which points at the larger gap: match() never compares an observation's quantity with the model's at all.
# observation labelled Water Level, model labelled Discharge
cmp = ms.match(obs, mr)
cmp.n_points # 10
cmp.quantity # Water Level [meter] <- the model's quantity is ignored entirely
cmp.skill() # scores it happily
The Comparer adopts the observation's quantity and never looks at the model's, so a mismatched comparison ends up mislabelled rather than merely wrong.
Suggested fix
- Treat both
"" and the legacy literal "Undefined" as undefined.
- Compare names on a normalised key (lowercased, non-alphanumerics stripped) so
"WaterLevel", "Water Level" and "water_level" agree. Names arrive from different vocabularies (EUM item names, CF long_name), and trivial spelling differences should not count as conflicts.
- Compare units only when both are non-empty, normalised the same way.
unit_display_name (src/modelskill/obs.py:768) already maps meter→m, second→s, degree→° and can be reused.
- Then wire it into
match() behind a check_quantity="error"|"warn"|"ignore" argument, matching the vocabulary of the existing obs_no_overlap. The default deserves a deliberate decision: strict catches the dangerous case, but risks false positives where the same physical quantity is named differently on the two sides.
The two rendered docstring examples on is_compatible keep their current results under this rule, so the generated docs are unaffected.
Quantity.is_compatible(src/modelskill/quantity.py:61) is meant to let an undefined quantity compare cleanly against anything. It does not, because it tests for the wrong sentinel.Two separate problems:
1. Wrong undefined sentinel. The method checks
self.name == "Undefined", butQuantity.undefined()returnsQuantity(name="", unit=""). Only the literal string"Undefined"is recognised, which is not what the constructor produces.2. Unit-strict. Beyond the undefined case it falls back to
self == other, so two quantities with the same name but different units never agree:This case is not hypothetical. Model results read from res1d and EPANET files carry a quantity name but no unit —
mikeio1dexposes quantities as a plain list of names — so a network model result is legitimatelyPressure []while the matching observation isPressure [MetresWater].Why it has gone unnoticed
is_compatibleis called from nowhere insrc/ortests/. Which points at the larger gap:match()never compares an observation's quantity with the model's at all.The Comparer adopts the observation's quantity and never looks at the model's, so a mismatched comparison ends up mislabelled rather than merely wrong.
Suggested fix
""and the legacy literal"Undefined"as undefined."WaterLevel","Water Level"and"water_level"agree. Names arrive from different vocabularies (EUM item names, CFlong_name), and trivial spelling differences should not count as conflicts.unit_display_name(src/modelskill/obs.py:768) already mapsmeter→m,second→s,degree→°and can be reused.match()behind acheck_quantity="error"|"warn"|"ignore"argument, matching the vocabulary of the existingobs_no_overlap. The default deserves a deliberate decision: strict catches the dangerous case, but risks false positives where the same physical quantity is named differently on the two sides.The two rendered docstring examples on
is_compatiblekeep their current results under this rule, so the generated docs are unaffected.