Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The SupportsSIUnitSystem property should no be needed #1463

Open
lipchev opened this issue Dec 16, 2024 · 3 comments
Open

The SupportsSIUnitSystem property should no be needed #1463

lipchev opened this issue Dec 16, 2024 · 3 comments

Comments

@lipchev
Copy link
Collaborator

lipchev commented Dec 16, 2024

You know how many of the quantities would throw an exception from their ToUnit/As(UnitSystem) method when they don't have any unit with a proper SI BaseUnit.. well I'd like to change that (at least in regards to UnitSystem.SI):

  • I think it's reasonable to say that no matter what unit system we use, "a dimensionless quantity should have the decimal fraction as it's base unit" - with the %, ppm, ppb etc. being the derived units. In the future we could probably talk about extending our definition of the BaseUnits (e.g. consider mg/mg as [M][M^-1] or something) but as long as the ratio is composed of the same base-units (i.e. no additional conversion such as x * 100) we could simplify the code generated for these methods to ignore the parameter (the unit system) and simply return the As(BaseUnit).
  • from memory, there are only a couple of other quantities which were failing (mostly due to simply missing their BaseUnits in the json-definition): I've already got these ready (as I needed them for something else)
  • from memory, the only breaking change w.r.t. the existing definitions was the Angle which used to have the DegreeCelsius as it's BaseUnit (which we've already fixed) and the FuelEfficiency, which I wanted to invert (I don't remember where I mentioned this), and for which I had to add the "SI unit" (don't quote me on that ) MeterPerCubicMeter (m/m³) (we could go even further and say that this should probably be it's BaseUnit but I haven't gone that far..😆)

Overall, I know that this can be done (as I've already removed the SupportsSIUnitSystem from my version of the tests, a few months ago..), so if you don't mind I'd like to do it for the v6.

PS In the future (or whenever you say), I'd also like to move the As(UnitSystem) and the ToUnit(UnitSystem) methods out of the IQuantity interface (similarly to what I propose in #1461 w.r.t. the "Equals with tolerance", these could be transformed into extension methods, without breaking the existing usages).

PS2 There is of course a lot more work to be done regarding the support for the other unit systems but I think we can keep the experimental flag on these for the v6 release, and come back to it later (I do have some ideas on the matter)

@lipchev
Copy link
Collaborator Author

lipchev commented Dec 20, 2024

You know how many of the quantities would throw an exception from their ToUnit/As(UnitSystem) method when they don't have any unit with a proper SI BaseUnit.. well I'd like to change that (at least in regards to UnitSystem.SI):

* I think it's reasonable to say that no matter what unit system we use, "a dimensionless quantity should have the decimal fraction as it's base unit" - with the `%`,  `ppm`, `ppb` etc. being the derived units. In the future we could probably talk about extending our definition of the `BaseUnits` (e.g. consider `mg/mg` as `[M][M^-1]` or something) but as long as the _ratio_ is composed of the same base-units (i.e. no additional conversion such as `x * 100`) we could simplify the code generated for these methods to ignore the parameter (the unit system) and simply return the `As(BaseUnit)`.

I almost forgot- with this proposal, I'd also like to remove the constructor that takes a UnitSystem from the dimensionless quantities:
public Ratio(double value, UnitSystem unitSystem)

@angularsen
Copy link
Owner

angularsen commented Dec 25, 2024

So in summary:

  • Change Ratio to support SI unit system, and use decimal fraction as its SI unit
  • Add BaseUnit for a few other quantities
  • Add FuelEfficiency.MeterPerCubicMeter to be SI compatible, and optionally make it its base unit
  • Move As(UnitSystem) and ToUnit(UnitSystem) out of IQuantity, make it an extension method instead (binary breaking change, source compatible)

I think this all sounds good, I'm fine doing this in v6 while you are on a roll.
Changing base unit should generally not be a huge breaking change, I consider it an internal implementation detail that external consumers should not rely on being permanent.

Regarding extension methods, make sure to put them in the UnitsNet namespace to be discoverable.
Also, I haven't thought through this, but we could maybe introduce ISiQuantity + ISiQuantityInfo interfaces for quantities that do have SI units, if it helps. Not sure what extra info I would expose at the top of my head though.

@lipchev
Copy link
Collaborator Author

lipchev commented Dec 25, 2024

So in summary:
* Move As(UnitSystem) and ToUnit(UnitSystem) out of IQuantity, make it an extension method instead (binary breaking change, source compatible)

Regarding extension methods, make sure to put them in the UnitsNet namespace to be discoverable. Also, I haven't thought through this, but we could maybe introduce ISiQuantity + ISiQuantityInfo interfaces for quantities that do have SI units, assuming there are quantities that simply don't have an SI unit.

We can do all but this part within the current v6, for the extension methods we'd have to wait for the Fractions/Unsafe version.

Even so, fixing the tests to work with the current interface definition would be quite helpful- it would at least remove (most) of the changes/merge conflicts we'd have in the tests (and improve the coverage- I already have the UnitSystem methods covered to 100%).

Furthermore, I won't even have to do all the changes to the UnitDefinitions in order for the potential PR to work- I've already made the relevant tests virtual, so I can just [Skip] the problematic quantities in the initial PR.

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
Projects
None yet
Development

No branches or pull requests

2 participants