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

Removing the FrequencyUnit.BUnit #1529

Merged
merged 1 commit into from
Mar 30, 2025

Conversation

lipchev
Copy link
Collaborator

@lipchev lipchev commented Mar 29, 2025

Removing the FrequencyUnit.BUnit.

As discussed in #1393 (comment)

@angularsen
Copy link
Owner

Just so I understand, we are removing this unit due to using Math.Sqrt?

In a way, I don't like the idea of removing something that someone actually requested and added, although yes it does seem very domain specific and I can't find much about this unit online.

I can get behind removing it for the reason that it is not widely used, but I have a feeling we'll need to support functions like Sqrt, Sin, Cos etc at some point anyway. Can we easily make exceptions for units like these?

@lipchev
Copy link
Collaborator Author

lipchev commented Mar 29, 2025

Just so I understand, we are removing this unit due to using Math.Sqrt?

In a way, I don't like the idea of removing something that someone actually requested and added, although yes it does seem very domain specific and I can't find much about this unit online.

I can get behind removing it for the reason that it is not widely used, but I have a feeling we'll need to support functions like Sqrt, Sin, Cos etc at some point anyway. Can we easily make exceptions for units like these?

The idea is that a unit conversions should be reversible, as such they cannot use irrational functions (I'm 99% sure there is format definition about it).

I don't see where it says that we need to support (now or in the future) any-type-of-expression as a valid unit for a quantity. In fact this, along with the FeetOfElevention (as a unit of pressure 😏) is currently the only use of a Math function in the unit definition (there are some uses of Math.PI but as long as it's defined as rational constant that's fine).

If you want the BUnit, it can be a property - but there is no way that it's gonna pass the equality tests.

@lipchev
Copy link
Collaborator Author

lipchev commented Mar 30, 2025

Here's another way of looking at this: having an expression such as value^2 would be the same as defining an AreaUnit for the Length quantity (or vice-versa). These in my opinion are clearly operations, not unit-conversions.

@angularsen
Copy link
Owner

Useful insight regarding operations vs unit conversions.

I don't know of any definitions regarding rules for only allowing rational unit conversions, can't find any with a quick google. Won't all logarithmic units be disallowed then?

I tried to think of examples with PI, but realize these are probably operations like you say, converting radius to area or circumference.

Anyway, we can remove this. Mostly because it is not widely used, maybe because it violates some unit definition rule, and probably because it is actually an operation and not a unit conversion.

@angularsen angularsen merged commit 3a62360 into angularsen:release/v6 Mar 30, 2025
1 check passed
@lipchev
Copy link
Collaborator Author

lipchev commented Mar 30, 2025

I don't know of any definitions regarding rules for only allowing rational unit conversions, can't find any with a quick google. Won't all logarithmic units be disallowed then?

The unit-conversion functions for the logarithmic quantities are still linear (x*a + b). The way we have it, this is also true for the multiplication and division, but not for the additions and subtractions.

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