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 BitRate and Information to double #1353

Merged
merged 14 commits into from
Feb 18, 2024

Conversation

Muximize
Copy link
Contributor

@Muximize Muximize commented Jan 6, 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.

@angularsen
Copy link
Owner

angularsen commented Feb 17, 2024

I say let's convert everything to double and see how it feels in v6. We have some time to test it out and gather feedback before releasing it as stable.

Next steps:

Rant, optional to read

Our current design simply feels half-baked and there is way too much complexity just to support decimal for 2-3 quantities that was added many years ago due to precision issues, and I believe those issues are now fixed.

The only quantity that has some value from decimal is Information, since bits are integer values and we don't want rounding errors on those, similar to TimeSpan.Ticks having type long, while all other properties are double TotalSeconds etc.

I'm sure we can find some reasonable solution to achieve the same thing, without going full QuantityValue. Maybe we can use long internally for this quantity and expose Information.Bits as long, while having all other properties as double similar to TimeSpan?

Anyway, we can solve this after converting to double. It is not a show stopper in my mind.

Brain dump of how to approach multiple numeric types

This is how I remember the different ways to approach multiple numeric types. We've had many a discussion on the topic.

  1. Change from struct to class to get inheritance, this primarily reduces binary size
  2. Use generics, like Length<decimal> + namespaced types UnitsNet.DecimalTypes.Length so you can import a certain namespace and get everything in the same value type
  3. Use QuantityValue and similar wrappers like now, holding the most common numeric types long, decimal, double, float. This may require nr 1, since the general recommendation for struct is to keep the size less than 16 bytes.
  4. Create separate nugets for different value types, e.g. UnitsNet using double, UnitsNet.Decimal using decimal etc. This may cause weird problems, like some libraries depending on double and others depending on decimal version of UnitsNet.

Some relevant sources

#714
#500
https://github.com/angularsen/UnitsNet/wiki/Experimental:-Generic-Math-and-INumber

@angularsen
Copy link
Owner

#1195 is now merged

@angularsen angularsen merged commit aeb0cd7 into angularsen:release/v6 Feb 18, 2024
1 check was pending
This was referenced Feb 18, 2024
@angularsen angularsen added this to the v6 milestone Feb 23, 2024
@Muximize Muximize deleted the bitrate-information-double branch February 24, 2024 16:15
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.

2 participants