Quantity.is_compatible reports undefined quantities as incompatible
- Dominant language
- Python
- Stars
- 56
- Forks
- 9
- Avg merge
- 57m
- Merged PRs (30d)
- 3
Description
`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.
```python
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:
```python
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.
```python
# 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.
Contributor guide
Research direction
Start with src/modelskill/quantity.py:61 and src/modelskill/obs.py:768 to understand the existing sentinel and unit-display normalization. Then trace match() and its obs_no_overlap option; done means undefined and normalized quantities compare correctly and match() exposes the requested quantity-check behavior without mislabeling mismatched results.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- data
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100