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

Decompose unit conversion functions to exact coefficients (non-breaking) #1498

Merged

Conversation

lipchev
Copy link
Collaborator

@lipchev lipchev commented Jan 19, 2025

This is a port of #1393, targeting the release-v6 consisting of the changes that didn't require any modifications to the tests.

I'll create another PR with the rest of the (breaking) changes in a bit.

Copy link

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

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.

I'll go ahead and merge this, just one minor comment/suggestion.

Tons of great work here 👏
I only skimmed the new constants though and spot tested a few places, but I have not verified every single change since it seems the tests still pass and that is a good indicator things are in order.

"FromUnitToBaseFunc": "{x} * Math.Pow(2.54e-2, 6)",
"FromBaseToUnitFunc": "{x} / Math.Pow(2.54e-2, 6)",
"FromUnitToBaseFunc": "{x} * 0.000000000268535866540096",
"FromBaseToUnitFunc": "{x} / 0.000000000268535866540096",
Copy link
Owner

Choose a reason for hiding this comment

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

In a way, I kind of prefer the Math.Pow formula, since it is easier to recognize the constant 2.54 and even repeating it 6 times 2.54e-2 * 2.54e-2 * ... could be one option if Math.Pow throws a curveball.
I have no strong opinion here, just an observation.

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 can work with Math.Pow as well, but if we (or whoever) wanted to created a generator for another language, the uses of Math means more hoops to deal with..

@angularsen angularsen force-pushed the unit-conversion-expressions-v6 branch from 0d23892 to e4f6e62 Compare March 23, 2025 17:14
@angularsen angularsen merged commit 5ea0939 into angularsen:release/v6 Mar 23, 2025
1 check was pending
@angularsen angularsen changed the title Decomposed the rounded expressions in the unit definitions into operations with exact coefficients (non-breaking) Decompose unit conversion functions to exact (non-breaking) Mar 23, 2025
@angularsen angularsen changed the title Decompose unit conversion functions to exact (non-breaking) Decompose unit conversion functions to exact coefficients (non-breaking) Mar 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants