-
Notifications
You must be signed in to change notification settings - Fork 382
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
Redesign UnitSystem to support non-SI systems and configurable default units #709
base: master
Are you sure you want to change the base?
Conversation
…stem - UnitSystem holds the list of default units for each (compatible?) quantity type and a method (GetDefaultUnitInfo) that provides the corresponding UnitInfo given a QuantityType - All methods in Quantity that use a UnitSystem now use the new GetDefaultUnitInfo - Added a method (in UnitSystem) for creating derived unit systems(WithDefaultUnit), providing a new QuantityType->UnitInfo association and optionally a new BaseUnits definition (as currently used by Equals and visible in public ctor- otherwise obsolete?)
Codecov Report
@@ Coverage Diff @@
## master #709 +/- ##
=========================================
+ Coverage 0 84% +84%
=========================================
Files 0 308 +308
Lines 0 43886 +43886
=========================================
+ Hits 0 36938 +36938
- Misses 0 6948 +6948
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Initially I went with a dictionary (as proposed in #651) but being stunned by the lack of improvement in performance compared with the test in #708 I dropped it in favor of an array (of QuantityInfo), indexed by the (int)QuantityType - 1, which gave me some improvement- but not nearly as much as I expected initially.. |
Note that the performance of my machine is slightly lower today (I think it's either the room temperature or some process eating up my resources). |
- updated the comments and invalid arguments checks - added a few tests for those cases a few question marks remain (marked with TODOs)
Still some things I'm not sure about:
|
We can't merge this into Other breaking changes: #666 |
I think this basically comes down to the fact that we have ~100 quantities today, and enumerating 100 items with a simple condition check is really really fast to begin with. At any rate, a lookup of sorts seems reasonable anyway. |
I propose using the default implementation that
I believe you are referencing this test: First, I don't understand why we are testing
I'll have to look closer on this one.
I'll have to look closer on this one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is work in progress, but here are some early comments after reading through the first time.
UnitsNet.Tests/UnitSystemTests.cs
Outdated
{ | ||
// TODO do we expect to preserve this behavior? | ||
// AmplitudeRatio might be unitless- but there are (more than one) ways to express ratios. | ||
Assert.Null(UnitSystem.SI.GetDefaultUnitInfo(AmplitudeRatio.QuantityType)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Preserve returning null if no mapping is defined.
- AmplitudeRatio should have a mapping defined in SI, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right- so, in theory, the only way to get a null here would be if one creates a custom unit system, explicitly setting the association to null (as presumable the original unit system would have all of its associations defined).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds about right. We should also create some unit tests that breaks as soon as a new quantity is added and if one forgets to add a mapping of default unit for included unit systems such as SI
and EnglishEngineering
. I don't think we need to add a mapping for default abbreviation, since I think it's natural to default to the first abbreviation defined in JSON as we already do.
UnitsNet.Tests/UnitSystemTests.cs
Outdated
[InlineData(LengthUnit.Meter, MassUnit.Kilogram, DurationUnit.Second, ElectricCurrentUnit.Ampere, TemperatureUnit.Undefined, AmountOfSubstanceUnit.Mole, LuminousIntensityUnit.Candela)] | ||
[InlineData(LengthUnit.Meter, MassUnit.Kilogram, DurationUnit.Second, ElectricCurrentUnit.Ampere, TemperatureUnit.Kelvin, AmountOfSubstanceUnit.Undefined, LuminousIntensityUnit.Candela)] | ||
[InlineData(LengthUnit.Meter, MassUnit.Kilogram, DurationUnit.Second, ElectricCurrentUnit.Ampere, TemperatureUnit.Kelvin, AmountOfSubstanceUnit.Mole, LuminousIntensityUnit.Undefined)] | ||
public void WithDefaultUnitThrowsIfSpecifiedBaseUnitsNotFullyDefined(LengthUnit length, MassUnit mass, DurationUnit time, ElectricCurrentUnit current, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, aren't we removing BaseUnits
?
UnitsNet/UnitSystem.cs
Outdated
/// <exception cref="ArgumentException"> | ||
/// Quantity type can not be undefined. | ||
/// </exception> | ||
public UnitSystem WithDefaultUnit(QuantityType quantityType, UnitInfo defaultUnitInfo, BaseUnits baseUnits = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BaseUnits is going away here too, right?
- renamed some tests, added comments here and there - WithDefaultUnit : added a check for the compatibility between QuantityType and UnitInfo passed in (also added a test for this)
- had missed this one
Regarding all comments about BaseUnits going away- you are right- they should be going away in v5. I just thought I might make this a two step PR (as to not make it too difficult to review)- where the first part would be the 'non-breaking' change (that allows for issue like the one described in #700 to be patched in the client code). However, now that I think of it- I'm not sure how good a fix is to pollute one's code with something like SI_Ex (having correctly mapped all necessary defaults by hand) just to have it fixed in the next major release (and going about changing back to SI once more)... |
Got it, good thinking, but I suspect the PR might be easier to review if we do a clean break to begin with instead of having both old and new designs in our head at the same time. It's up to you, just do whatever you feel makes the most sense. |
- added an association (1:0..1) between UnitSystem and Unit (kept the name 'BaseUnit' instead of the previously considered 'DefaultUnit') - added an association (1:0..n) between Unit and UnitSystem- this would be used to generate list of units for a quantity in a particular unit system (not an exhaustive list, needs a review)
Here is an example of the modified unit definition schema (haven't removed the BaseUnits from the CubicHectometer & ImperialPint as to not trigger the code-regeneration)- tell me what you think. The two associations would be used to construct the corresponding list of UnitInfo's and default UnitInfo for a given quantity and unit system. Modifying the whole list of UnitDefinitions in such a way would be quite the task- but as long as there is some review process- I think we can manage. As we do that- what I think would be really cool is to associate units & quantities with their actual ontological references: like for instance our MassFraction is defined in UO as MassPercentage and again as MassFraction in OM. We could thus (maybe in the future) run some ontological alignment validations on our unit definitions, as described in this awesome paper: Comparison and Evaluation of Ontologies for Units of Measurement. Not to mention the nice extension methods that one could think of.. |
I like it. Explicit and much simpler to reason about than before. I originally thought about defining unit system as its own file, but keeping it per quantity is actually a much better idea since it's much more visible and just a single JSON file to edit when adding new units. It's still something new that contributors will have to think about, but with some updated steps in the wiki I think we should manage. Also, unit systems are really opt-in the way I see it. Meaning, if we decide to not map all units they should still continue to work for everything else but methods that take a unit system as argument. |
- refactored the UnitSystem- moving the BaseUnits to BaseUnitSystem (SI derived from BaseUnitSystem), extended the unit mappings with the UnitSystemInfo class (containing both default and common/derived units) - updated the JsonTypes and created the UnitSystemInfoGenerator (TODO WRC) - updated existing UnitSystemTests that tested for equality/BaseUnits (almost all in fact) to use the BaseUnitSystem instead (as the IEquatble/BaseUnits interface was pushed down) - added several more unit-system mappings to the UnitDefinitions (Acceleration, AmountOfSubstance, Area, Energy & Length) - commented (temporary) part of the Ctor_WithValueAndSIUnitSystem_ReturnsQuantityWithSIUnitOrThrowsArgum test (testing for support SI support)
Ok, so I refactored UnitSystem- I couldn't bear to remove the BaseUnits so I moved it the BaseUnitSystem extension of the UnitSystem (along with the IEquatable interface). It is thus still present for UnitSystem.SI (which is a BaseUnitSystem)- I don't yet know how we could is use- but I don't think it hurts to keep it for now. I've also left out the original BaseUnits constructor (and the corresponding mapping behavior)- marked as obsolete. Thus it seems we are mostly backward-compatible. (still I presume we will move this to v5?) I've replaced the previous a UnitInfo[] mapping (in the UnitSystem) with a UnitSystemInfo- that includes the default unit (optional) and list of "common" (also "derived" or "named") units for a quantity in a unit system. This class itself is currently not exposed (maybe make internal)- only the UnitInfos are exposed via the GetDefaultUnitInfo(QuantityType quantityType) & GetCommonUnitsInfo(QuantityType quantityType) methods in the UnitSystem. Next I updated the JSON types & added UnitSystem generator- that completes the partial part of the concrete UnitSystem implementations (SI, GGS, EE etc..) and finally added several more UnitSystem mappings to the UnitDefinitions of Acceleration, AmountOfSubstance, Area, Energy & Length (pretty much the same thing as with Volume- note however that CGS often maps default unit to prefixed-quantities). Still some tests need to be added (note for instance how it's not really possible to argument check the Lazy in the constructor). Also I've commented(temporary) parts of the Ctor_WithValueAndSIUnitSystem_ReturnsQuantityWithSIUnitOrThrowsArgum, and two of my own tests for the WithDefaultUnit registration- that used to test for the BaseUnit- haven't created an overload for it in BaseUnitSystem yet) |
Also- the benchmark showed some unexpected results- I didn't expect to see any change over the previous tests (we are still doing the same array lookup) - yet the results turned up even faster than the original As/ToUnit methods- what, am I not doing enough argument checking somewhere? Or maybe it is something worse :) |
|
I've kind of forgotten to follow up on this. Will try to get to it sometime soon. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
I added the |
- UnitSystem/BaseUnitSystem Equality contract based on the base (default) unit for each quantity - BaseUnits for BaseUnitSystem created from the provided unit associations - added unit system associations for all SI base types (Length, Mass, Duration etc.) - tests for the UnitSystem construction/derivation
- updated the default units (as per https://en.wikipedia.org/wiki/Astronomical_system_of_units) - fixed the docstring
in order to avoid breaking the namespace (UnitSystem.SI) all static instances were added to the UnitSystem.Definition (partial class)
- fixed typo in unit system for Length - Ctor_UnitSystem_ThrowsArgumentExceptionIfNotSupported - Ctor_WithNullUnitSystem_ThrowsArgumentNullException - As_UnitSystem_ThrowsArgumentExceptionIfNotSupported - As_WithNullUnitSystem_ThrowsArgumentNullException - To_UnitSystem_ThrowsArgumentExceptionIfNotSupported - ToUnit_WithNullUnitSystem_ThrowsNullException
- merged all upstream modifications - fixed nullability issues in UnitSystem - the Lazy<..> BaseUnits constructor was made protected - marked all tests that rely solely on BaseUnits as Obsolete (with error=false) - removed some obsolete tests from MassConcentration (tesing for default unit system units)
This is almost done- still need to complete mapping in the JSON files, but as it stands I can only see one client call that is breaking : I've kept the equality contract for BaseUnitSystem as before- only comparing the default SI base units, where as for all other unit systems (not derived from BaseUnitSystem) we compare all unit associations. I've added tests akin to the ones you've suggested in #844. |
Awesome, I'll try to take a closer look this weekend. |
@lipchev I have totally neglected this one, but stumbled over it today. Would you like me to review this still? |
Also, related #864. Here we deprecate QuantityType in favor of strings, which is more extensible. It seems like something this PR should take into account as well. |
I would suggest the naming the alternative Unit System of "EnglishEngineering" units be defined as "USCustomary" as I believe that is the proper name. I believe the "English" or "Imperial" is historical naming and not used anymore. |
Thank you @inflectionpoint , I didn't know that! |
…mentExceptionIfNotSupported tests
@angularsen I've merged everything from upstream, removing the SupportsSIUnitSystem tests in favor of the auto-generated equivalents. |
@lipchev I think the reasoning for Undefined was to catch bugs where someone didn't specify a value, such as when deserializing JSON. |
where is the run button!?
This reverts commit e027e7c.
…UnitSystemWithOldDerivedUnits_IncludingTheNewBaseUnit
I don't know where to put this (maybe a discussion?) but here's a great explanation of the linear algebra behind the BaseUnits of different UnitSystems. |
(WIP) for #651
1. Redesign/Remove BaseUnits type and remove related from JSON and UnitSystem (breaking change)
2. UnitSystem: Add mapping of default unit, such as QuantityType.Length => LengthUnit.Centimeter
3. UnitSystem: Add mapping of default abbreviation per unit enum value, such as LengthUnit.Feet => "ft" (optional- also a breaking change if UnitAbbreviationCache is to become part of the UnitSystem)
4. Length.ctor(double, UnitSystem) constructs with unit system's default unit for Length
5. double val = myLength.As(UnitSystem) converts to unit system's default unit for Length
6. UnitSystem should remain immutable, so to change its configuration at runtime you create a new instance either manually or by cloning an existing system with extension methods for adding/modifying mappings like in use case 5. Then you inject this instance into your view or whatever.
7. Add UnitSystem.SI and UnitSystem.EnglishEngineering with mappings
8. Add tests for the the new public methods in UnitSystem (got one in LenghtTests- but none that test for the "exceptional" cases)