Skip to content

Is there a reason, the conversion properties return double for quantities with base type decimal? #1058

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

Closed
pgrawehr opened this issue Mar 3, 2022 · 10 comments
Labels
enhancement pinned Issues that should not be auto-closed due to inactivity.

Comments

@pgrawehr
Copy link
Contributor

pgrawehr commented Mar 3, 2022

Some quantities, for instance information have decimal as base type (which does make sense). However, except for the Value property, all properties return double:

/// <summary>
///     Gets a <see cref="double"/> value of this quantity converted into <see cref="InformationUnit.Bit"/>
/// </summary>
public double Bits => As(InformationUnit.Bit);

/// <summary>
///     Gets a <see cref="double"/> value of this quantity converted into <see cref="InformationUnit.Byte"/>
/// </summary>
public double Bytes => As(InformationUnit.Byte);

This is a bit confusing and unnecessarily looses precision. Would it be possible to change these properties to decimal, too?

@tmilnthorp
Copy link
Collaborator

tmilnthorp commented Mar 3, 2022

I agree it's not great.

We just closed #1051 which would have deprecated the double conversion properties in favor of returning a Quantity type. You would then be able to use the Value property (or IConvertible interfaces) to retrieve the result as a decimal.

Feel free to look over the arguments there, but I do feel that returning a Quantity gives you more power/flexibility in terms of what you can do with the result.

@lipchev
Copy link
Collaborator

lipchev commented Mar 3, 2022

There is also #875 for you (all) to consider- it's not the most straightforward approach and I haven't thought it through a 100% - but that's why I put it up for discussion.

@tmilnthorp
Copy link
Collaborator

Although not as convenient, you can also use

BitRate rate = BitRate.FromKilobitsPerSecond(56);
decimal mb = rate.ToUnit(BitRateUnit.MegabitPerSecond).Value;

@angularsen
Copy link
Owner

Closing this in favor of #875

@angularsen
Copy link
Owner

Re-opening, realized this describes one of several solutions discussed in #875 .

  • IQuantity.Value should return QuantityValue
  • decimal based quantities should return decimal in their conversion properties, like Information.Bits

@stale
Copy link

stale bot commented Aug 13, 2022

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 Aug 13, 2022
@angularsen
Copy link
Owner

Not stale, PR #1074 recently had activity.

@stale stale bot removed the wontfix label Aug 14, 2022
@stale
Copy link

stale bot commented Nov 12, 2022

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 Nov 12, 2022
@pgrawehr
Copy link
Contributor Author

Not stale, PR still up for review.

@stale stale bot removed the wontfix label Nov 15, 2022
@angularsen angularsen added the pinned Issues that should not be auto-closed due to inactivity. label Nov 17, 2022
angularsen added a commit that referenced this issue Nov 29, 2022
Fixes #1058 

Return the correct value type for quantities that use `decimal` internally; `Power, Information, BitRate`.

- Quantity properties return `decimal` or `double` based on internal value type.
- `IQuantity.Value` returns `QuantityValue`, which supports both double and decimal.
- `QuantityValue`: Implement IEquality<QuantityValue>, IComparable<QuantityValue>, IComparable.

Co-authored-by: Andreas Gullberg Larsen <andreas.larsen84@gmail.com>
@angularsen
Copy link
Owner

Fixed by #1074

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement pinned Issues that should not be auto-closed due to inactivity.
Projects
None yet
Development

No branches or pull requests

4 participants