Adding BaseUnits for Angstrom results in test failures due to UnitSystem logic #1043
Comments
tmilnthorp
added
bug
pinned
Merged
lipchev
added a commit
to lipchev/UnitsNet
that referenced
this issue
Dec 30, 2024
|
@angularsen I've just added the |
|
Very nice 👏 |
lipchev
added a commit
that referenced
this issue
Dec 30, 2024
Fixes #1463 Fixes #1043 - removed the `UnitSystem` constructor from the Dimensionless quantities (which was previously throwing) - `As`/`ToUnit(UnitSystem)` for all dimensionless quantities now convert to their `BaseUnit` (i.e. the "DecimalFraction") * - `As/ToUnit(UnitSystem)` for all other quantities refactored using the QuantityInfoExtensions - added tests for the `UnitSystem` methods, skipping the tests for all quantities that fail with `UnitSystem.SI` (with a reason) There are only two dimensionless quantities (IMO) that don't fit the definition: - `RelativeHumidity`: currently has only the `Percent` unit, I think we should add the `DecimalFraction`, setting it to be the `BaseUnit` - `FuelEfficiency`: I think this could be defined as `"L": -2` with the addition of the `MeterPerCubicMeter` unit (possibly setting it as its `BaseUnit`, if we want to satisfy the `BaseUnit_HasSIBase` test) You can look for `As_UnitSystem_ReturnsValueInDimensionlessUnit` if you want to check the rest of the dimensionless quantities. Regarding the removed `BaseUnits` (see `Force.json` or `Pressure.json`), those were detected by some earlier tests I had in place, regarding the multiplication/division operators where I used the following definition: - A given operation between two quantities (either multiplication or division) such as `A / B = C` is only defined if `A.Dimensions / B.Dimensions = C.Dimensions` - When the intersection between `A.Dimensions` and `B.Dimensions` is the empty set, for every unit of `A` and `B` for which the `BaseUnits` is not `Unidefined`, and every unit of `C`, having `BaseUnits` = `A.BaseUnits union B.BaseUnits`, it must be true that `C.Value = A.Value / B.Value`. - When the intersection between `A.Dimensions` and `B.Dimensions` is not empty, and the intersecting `BaseUnits` of `A` , `B` and `C` are all the same, then again we have the same condition, which I generally refer to as "the conversion coefficient is 1" - The same logic can be used to infer the unit-conversion coefficient based off the `Dimensions` and a pair of `BaseUnits`, but special attention needs to be taken w.r.t. the exponents (e.g. `Area` is `L2` so the unit-conversion coefficients are squares of the ones from `Length`) - If we want to extend this definition in the future, we should consider introducing a way to override the "default conversion coefficient" (`1`).. Here are some links: https://en.wikipedia.org/wiki/Dimensional_analysis https://en.wikipedia.org/wiki/International_System_of_Units#Definition https://en.wikipedia.org/wiki/Coherence_(units_of_measurement)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
Adding BaseUnits for Angstrom results in test failures due to UnitSystem logic. The BaseUnits for Armstrongs are coming as the first QuantityInfo in the UnitSystem constructor since A happens to be first in the alphabetic list.
To Reproduce
Steps to reproduce the behavior (just an example):
Expected behavior
Meters is still the expected LengthUnit for SI.
Screenshots
The text was updated successfully, but these errors were encountered: