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

Power is using the decimal type #1194

Open
tmilnthorp opened this issue Feb 8, 2023 · 6 comments · Fixed by #1195
Open

Power is using the decimal type #1194

tmilnthorp opened this issue Feb 8, 2023 · 6 comments · Fixed by #1195

Comments

@tmilnthorp
Copy link
Collaborator

I'm upgrading to 5.1.0 in my projects and noticed that Power is using the decimal type.

I don't think this is a bug so much as questioning if this is expected? It has been this way since 2014, I only just noticed since the value property in v5 now returns the underlying type rather than always double.

I would expect this type to be consistent with much of the other types: double.

Per the PR:

I was thinking about this previously and I think decimal might be a better backing datatype because it can exactly represent powers of 10 because it's a decimal floating point type (double is binary floating point). It would make sense to use it since we're mainly concerned with SI units which are powers of 10, however it's a major breaking change and decimal is less efficient for calculations...

I don't think this is applicable anymore given the test checking within a tolerance. Let me make a PR and we can discuss,.

@angularsen
Copy link
Owner

angularsen commented Feb 8, 2023

I'm all for it, double down on double.
BitRate, Information and Power are the types using decimal now. 3 out of 100+. It doesn't make any sense.

The only concern is breaking changes that requires another major version bump and that integer quantities like Information may start getting rounding errors it did not previously have.

Even so, a consistent type is better in the end and I would rather look into generics to support decimal, float and other number types if anyone cares enough about it to champion it.

@Muximize
Copy link
Contributor

+1, the breaking changes with Power are preventing us from upgrading to v5 without lots of casting. I'd like to applaud @tmilnthorp for his PR efforts.

@tmilnthorp
Copy link
Collaborator Author

I'd like to applaud @tmilnthorp for his PR efforts.

Thanks!

@angularsen
Copy link
Owner

@Muximize #1205 was resolved by #1207 , can you take it for a spin if this unblocks you?

Changing Power to double will be a breaking change, and you'll have to wait for v6 prerelease nugets for that. No ETA yet. Tracked by #1195 .

@Muximize
Copy link
Contributor

Alas, we're using the conversion properties in quite a few places so we have to wait for v6.

I should have paid attention and protest against this change landing in v5 in the first place 😅

@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 added a commit that referenced this issue Feb 18, 2024
Fixes #1194

---------

Co-authored-by: Andreas Gullberg Larsen <andreas.larsen84@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants