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

Adding the missing BaseUnits #1473

Open
wants to merge 2 commits into
base: release/v6
Choose a base branch
from

Conversation

lipchev
Copy link
Collaborator

@lipchev lipchev commented Dec 27, 2024

Fixes #1463

  • 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)

- removed the UnitSystem constructor from the Dimensionless quantities (which was previously throwing)
- As/ToUnit(UnitSystem) for all Dimensionsless 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)
@lipchev
Copy link
Collaborator Author

lipchev commented Dec 27, 2024

@angularsen I initially thought about not touching the unit definitions, and just [Skip] the tests - but after seeing that the tests failed for 61 quantities, decided that it would be simpler to just add the BaseUnits, rather than add to 61 test files (just to remove the addition in the next PR).

Hopefully you wont find the json files too difficult to review..

PS In #1463 I had totally forgotten that the generator does not (yet) handle the BaseUnits for the prefixes. I wrote some code (long ago) that deals with the issue, but it's a lot, so I decided to leave it out of this PR. Once this is merged, I'll create another PR that deals with that issue alone (which should clear all the [Fact(Skip = "The BaseUnits are not yet supported by the prefix-generator")].

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got a bunch more extensions in here, for the time being they are internal- with one two good candidates for being public.
Anyway- tell me if you want to move them to another location / namepsace..

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The property is no longer used (other than the overrides) - but I didn't want to remove it just yet, as I wanted to only have the failing tests show up in this PR.
Once this is merged, I'll create another one that just removes the property.

@lipchev
Copy link
Collaborator Author

lipchev commented Dec 27, 2024

I took some other notes while preparing this PR:

The initial size for UnitsNet.dll was

2.23 MB (2 339 328 bytes)

  1. After adding the extension and refactoring the constructor / As/To(UnitSystem) methods (with 61 quantities failing due to missing SI BaseUnit)

2.20 MB (2 311 680 bytes)

  1. After adding all the missing BaseUnits

2.21 MB (2 321 408 bytes)

Still ahead, but if we include the "prefixed" base units- things will likely even out (possibly even setting us back some).

PS Finally, here is how this would look with the prefix-generation enabled:

2.23 MB (2 346 496 bytes)

@lipchev
Copy link
Collaborator Author

lipchev commented Dec 27, 2024

Also noted the coverage 😄 :

Information
Parser: MultiReport (3x DotCover)
Assemblies: 3
Classes: 293
Files: 298
Line coverage
85%
Covered lines: 30789
Uncovered lines: 5179
Coverable lines: 35968
Total lines: 166435
Line coverage: 85.6%

And now:

Information
Parser: MultiReport (3x DotCover)
Assemblies: 3
Classes: 294
Files: 299
Line coverage
88%
Covered lines: 30364
Uncovered lines: 3765
Coverable lines: 34129
Total lines: 163888
Line coverage: 88.9%

@lipchev
Copy link
Collaborator Author

lipchev commented Dec 27, 2024

Oh, forgot about the tests in IQuantityTests - the ones that are testing the UnitSystem methods are now redundant, but we can remove them as we complete the coverage of the As/ToUnit methods..

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one wasn't re-generated from the last commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant