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

Convert Power to double #1195

Merged
merged 5 commits into from
Feb 18, 2024
Merged

Conversation

tmilnthorp
Copy link
Collaborator

@tmilnthorp tmilnthorp commented Feb 8, 2023

Fixes #1194

@tmilnthorp
Copy link
Collaborator Author

Fails in json tests. Unsure what to do here.

@angularsen
Copy link
Owner

angularsen commented Feb 8, 2023

3 tests failing in CI it seems, all related to JSON converters. We probably handpicked decimal quantities for these tests, but we would have to create a new dummy quantity for testing purposes I guess if we want to keep the decimal JSON functionality around.

It would also be important to verify that old serialized JSON for Power/BitRate/Information can still be deserialized after switching to double.

image
https://dev.azure.com/unitsnet/Units.NET/_build/results?buildId=311&view=logs&j=3dc8fd7e-4368-5a92-293e-d53cefc8c4b3&t=9d4476d9-f6e2-5f97-1036-4124d48c41e1

@tmilnthorp
Copy link
Collaborator Author

3 tests failing in CI it seems, all related to JSON converters. We probably handpicked decimal quantities for these tests, but we would have to create a new dummy quantity for testing purposes I guess if we want to keep the decimal JSON functionality around.

It would also be important to verify that old serialized JSON for Power/BitRate/Information can still be deserialized after switching to double.

image https://dev.azure.com/unitsnet/Units.NET/_build/results?buildId=311&view=logs&j=3dc8fd7e-4368-5a92-293e-d53cefc8c4b3&t=9d4476d9-f6e2-5f97-1036-4124d48c41e1

Yea the tests were just using Power to test decimal types. I just convered them to another existing decimal type.

Def should add a test to test old output.

@angularsen angularsen added this to the vNext milestone Feb 17, 2023
@tmilnthorp tmilnthorp changed the base branch from master to release/v6 February 24, 2023 12:40
Copy link
Owner

@angularsen angularsen left a comment

Choose a reason for hiding this comment

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

Looks good, just a few minor suggestions.

Question; what do you think of also changing Information/BitRate from decimal to double?

On one hand, it is sort of weird to risk getting rounding errors like 8.00000001 bits. On the other hand, for all other units (kilobyte, etc) that are probably way more used, floating point is perhaps the expected value representation.

If we change all 3 quantities to double, we have the potential to clean up a LOT of QuantityValue complexity.

UnitsNet.Tests/DecimalOverloadTests.cs Outdated Show resolved Hide resolved
UnitsNet.Tests/IntOverloadTests.cs Outdated Show resolved Hide resolved
@tmilnthorp
Copy link
Collaborator Author

Question; what do you think of also changing Information/BitRate from decimal to double?

As much as it pains me, I think these actually do make sense as decimal by default. If we had generic support, it could be whatever the client wishes.

@angularsen
Copy link
Owner

angularsen commented Mar 4, 2023

I'm still not sure about Information, there's a lot of code just to avoid rounding errors on the unit Bit. For all other units Kilobyte etc, a non-integer value is probably expected anyway.

Although, with #1206 it would probably read 5.299999999 kB instead of 5.3 kB in some cases.
But, with #1206 we kind of force everyone to use ToString("s2") anyway, and that would return 5.30 kB.

Come to think ofit, isn't s2 almost equal to n2? Or actually, more like 0.## custom format.
Do we need s?

Here tested with current implementation in master, where g equals our special implementation rounding to 2 decimals.

CultureInfo.CurrentCulture = CultureInfo.InvariantCulture;
1000.2.ToString("g2").Dump();                    // 1e+03
1000.2.ToString("n2").Dump();                    // 1,000.20
Length.FromFeet(1000.2).ToString("s2").Dump();   // 1,000.2 ft
Length.FromFeet(1000.2).ToString("g2").Dump();   // 1,000.2 ft
Length.FromFeet(1000.2).ToString("n2").Dump();   // 1,000.20 ft
Length.FromFeet(1000.2).ToString("f2").Dump();   // 1000.20 ft

1000.234.ToString("g2").Dump();                  // 1e+03
1000.234.ToString("n2").Dump();                  // 1,000.23
Length.FromFeet(1000.234).ToString("s2").Dump(); // 1,000.23 ft
Length.FromFeet(1000.234).ToString("g2").Dump(); // 1,000.23 ft
Length.FromFeet(1000.234).ToString("n2").Dump(); // 1,000.23 ft
Length.FromFeet(1000.234).ToString("f2").Dump(); // 1000.23 ft

@stale
Copy link

stale bot commented Jun 18, 2023

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.

@stale stale bot added the wontfix label Jun 18, 2023
@angularsen angularsen added the pinned Issues that should not be auto-closed due to inactivity. label Jul 11, 2023
@stale stale bot removed the wontfix label Jul 11, 2023
@ebfortin
Copy link
Contributor

Looks good, just a few minor suggestions.

Question; what do you think of also changing Information/BitRate from decimal to double?

On one hand, it is sort of weird to risk getting rounding errors like 8.00000001 bits. On the other hand, for all other units (kilobyte, etc) that are probably way more used, floating point is perhaps the expected value representation.

If we change all 3 quantities to double, we have the potential to clean up a LOT of QuantityValue complexity.

I agree. Floating point error is a fact of life anyway. Some number can't even be represented and we still use them for computation everywhere. I don't think it's worth the complexity just for one unit.

# Conflicts:
#	UnitsNet.Tests/CustomCode/PowerTests.cs
#	UnitsNet.Tests/GeneratedCode/TestsBase/PowerTestsBase.g.cs
#	UnitsNet/GeneratedCode/Quantities/Power.g.cs
No more decimal quantities support UnitSystem after removing Power.
The remaining 2 decimal quantities Information and BitRate don't support UnitSystem, and we also plan to change them from decimal to double.
@angularsen angularsen merged commit 7e7053e into angularsen:release/v6 Feb 18, 2024
1 check passed
angularsen added a commit that referenced this pull request Feb 18, 2024
In PR #1195 @angularsen asks @tmilnthorp:
> Question; what do you think of also changing Information/BitRate from
decimal to double?
> 
> On one hand, it is sort of weird to risk getting rounding errors like
8.00000001 bits. On the other hand, for all other units (kilobyte, etc)
that are probably way more used, floating point is perhaps the expected
value representation.
> 
> If we change all 3 quantities to double, we have the potential to
clean up a LOT of QuantityValue complexity.

How about we continue that discussion here? 😄 

This PR is pretty straightforward and mimics #1195, except that
`Information` is used as the typical decimal quantity in some tests.
Because there won't be any decimal quantities left if these PRs get
merged, I removed those tests in anticipation of completely removing all
decimal support.

---------

Co-authored-by: Andreas Gullberg Larsen <andreas.larsen84@gmail.com>
angularsen added a commit that referenced this pull request Feb 23, 2024
### Changes
- Remove `QuantityValue`, replaced with `double`
- Remove `TValueType` from interfaces
	- Remove `IQuantity<TUnitType, out TValueType>`
	- Remove `IValueQuantity<out TValueType>`
- Change `IQuantity<TSelf, TUnitType, out TValueType>` to
`IQuantity<TSelf, TUnitType>`
- Change `IArithmeticQuantity<TSelf, TUnitType, TValueType>` to
`IArithmeticQuantity<TSelf, TUnitType>`

### Changes to UnitsNet.Serialiation.JsonNet
- Deserializing previously serialized JSON for decimal quantities
`Information`, `BitRate` and `Power` still work, but it now reads just
`double Value` property and ignores `string ValueString` and `string
ValueType` properties. This may lose precision compared to preserving
the full `decimal` value, but `decimal` is no longer supported in v6.


### Background
In #1195 @angularsen says:

> If we change all 3 quantities to double, we have the potential to
clean up a LOT of QuantityValue complexity.

This made me wonder how deep that complexity goes so I decided to
experiment, and this is the result.

I must say some of these changes make me a bit sad. A lot of work and
some very clever thinking went into supporting multiple numerical types,
and I want to acknowledge that. 🙇

Also, I took it as far as possible but that might not be the best
outcome, for example we might want to keep deserialization support. This
just demonstrates a possible direction we could go in.

---------

Co-authored-by: Andreas Gullberg Larsen <andreas.larsen84@gmail.com>
@angularsen angularsen mentioned this pull request Feb 23, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change pinned Issues that should not be auto-closed due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Power is using the decimal type
3 participants