-
Notifications
You must be signed in to change notification settings - Fork 2
feature: Fractional Exponents #7
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
base: main
Are you sure you want to change the base?
feature: Fractional Exponents #7
Conversation
NeedleInAJayStack
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work!! I love this approach.
Just wondering - have we validated that exponents in a fractional form are sufficient? Like that defining units in a decimal-based format is not required/desired? Also worth noting (although it's probably completely irrelevant), is that integer fractions cannot represent all decimal values: https://en.wikipedia.org/wiki/Irrational_number
Sources/Units/Unit/Fraction.swift
Outdated
|
|
||
| var positive: Bool { | ||
| switch (numerator, denominator) { | ||
| // 0/0 is not positive in this logic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as an aside, here's a fun math Wikipedia article on this particular fraction: https://en.wikipedia.org/wiki/Indeterminate_form#Indeterminate_form_0/0
| } | ||
|
|
||
| extension SignedInteger { | ||
| func over<T: SignedInteger>(_ denominator: T) -> Fraction { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were there type inference issues with using /, like 5.measured(in: .meter.pow(1/2))?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. That was what I originally tried.
It was ambiguous to the compiler whether 1 / 2 meant Int / Int or Fraction / Int or Fraction / Fraction etc because Fraction conforms to ExpressibleAsIntegerLiteral.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On that note, how do you feel about | as the operator? It wouldn't be ambiguous.
You could do 2|5 etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and did this - lmk if you like/dislike.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. That was what I originally tried.
It was ambiguous to the compiler whether 1 / 2 meant Int / Int or Fraction / Int or Fraction / Fraction etc because Fraction conforms to ExpressibleAsIntegerLiteral.
Ah, gotcha. Yeah, sounds good to me.
On that note, how do you feel about | as the operator? It wouldn't be ambiguous.
For the Swift API, I don't really like it. It causes inheriting packages to require type declarations on any integer bitwise OR which can be pretty annoying (we can see this in the new FractionTests, where you had to do some gymnastics to get type declarations in there). Honestly, I'd prefer just using over and no operator instead of providing some syntax sugar that breaks other stuff.
For a equation/string representation, I'd like to use / if possible, but if not then | seems okay.
Fractional form is not sufficient. It's just a step in the right direction. I haven't thought much about how we'll straddle the operation between |
bfa5aac to
53a7872
Compare
Oof, do you think we should continue down this direction then or just try and go toward a 'double'-ized exponent? I'd hate to introduce a |
53a7872 to
03652e7
Compare
Sorry it won't be subsuming the functionality.
The reason we need fraction parts is to preserve precision for as long as possible so we don't run into a case like Can you think of any reason I wouldn't need to worry about that and could instead just convert all fractional exponents into Doubles and still get the right behavior during dimensional analysis? The main use case is for intermediate units that when combined reduce into units that make sense. |
|
|
||
| XCTAssertEqual( | ||
| try Unit(fromSymbol: "m^(2|5)"), | ||
| Unit.meter.pow(2.over(5)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would Unit.meter.pow(2|5) compile with this API? As an alternative to .over
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd have to overload the | operator which I am rather wary of doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh lol it looks like that's already what I did... haha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that.. I think it makes intuitive sense for the digital twin developers who unfortunately write equations as strings that will need to be compiled. It's more intuitive for them to write "pow(2|5)" as a string than "pow(2.over(5))" as a string.
In the conversation with Jay it seems like you also tried to support an API that allows pow(2/5), but this lead to some type inference issues? I think this would be ideal, but if that's not possible "|" seems appropriate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I give my take here: #7 (comment)
TLDR: Overloading | doesn't seem like a great option to me either, since it has the same Ambiguous use issues as /, except with bitwise or instead of divide.
wyasul
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need this! Looks good.
03652e7 to
87a14f4
Compare
NeedleInAJayStack
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for bringing this back up. I've added a few more requests.
| /// - Parameter raiseTo: The exponent to raise the measurement to | ||
| /// - Returns: A new measurement with an exponentiated scalar value and an exponentiated unit of measure | ||
| public func pow(_ raiseTo: Int) -> Measurement { | ||
| public func pow(_ raiseTo: Fraction) -> Measurement { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid a breaking change, could we just add a new function that takes Fraction, but keep the old one too? I think this would also resolve the current compilation failures.
| /// Returns a list of defined units and their exponents, given a composite unit symbol. It is expected that the caller has | ||
| /// verified that this is a composite unit. | ||
| func compositeUnitsFromSymbol(symbol: String) throws -> [DefinedUnit: Int] { | ||
| internal func compositeUnitsFromSymbol(symbol: String) throws -> [DefinedUnit: Fraction] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Remove explicit internal
| self.denominator = denominator / gcd | ||
| } | ||
|
|
||
| public var positive: Bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have Comparable conformance, do we need this? Like, couldn't we just compare an instance to zero (i.e. fraction > 0)? It looks like this function is only used within description and tests.
| /// - Parameter subUnits: A dictionary of defined units and exponents. If this dictionary has only a single unit with an exponent of one, | ||
| /// we return that defined unit directly. | ||
| init(composedOf subUnits: [DefinedUnit: Int]) { | ||
| internal init(composedOf subUnits: [DefinedUnit: Fraction]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Remove internal
| name: String, | ||
| symbol: String, | ||
| dimension: [Quantity: Int], | ||
| dimension: [Quantity: Fraction], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same deal on all the public API changes in this file - it'd be great to make new functions instead of changing existing ones so that this isn't a breaking change. I'd expect most users to use Int anyway, since whole exponents are much more common than fractional.
| if denominator == 1 { | ||
| "\(!positive && numerator != 0 ? "-" : "")\(abs(numerator))" | ||
| } else { | ||
| "(\(positive ? "" : "-")\(abs(numerator))|\(abs(denominator)))" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that it isn't possible to overload the / Swift operator, but I think we could use it in the string representation here, right? As long as we require fractional exponents to be surrounded by parentheses, I think we can force valid parsing using /.
| } | ||
|
|
||
| extension SignedInteger { | ||
| func over<T: SignedInteger>(_ denominator: T) -> Fraction { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. That was what I originally tried.
It was ambiguous to the compiler whether 1 / 2 meant Int / Int or Fraction / Int or Fraction / Fraction etc because Fraction conforms to ExpressibleAsIntegerLiteral.
Ah, gotcha. Yeah, sounds good to me.
On that note, how do you feel about | as the operator? It wouldn't be ambiguous.
For the Swift API, I don't really like it. It causes inheriting packages to require type declarations on any integer bitwise OR which can be pretty annoying (we can see this in the new FractionTests, where you had to do some gymnastics to get type declarations in there). Honestly, I'd prefer just using over and no operator instead of providing some syntax sugar that breaks other stuff.
For a equation/string representation, I'd like to use / if possible, but if not then | seems okay.
| XCTAssertTrue((3 | 5).positive) | ||
| XCTAssertTrue((-4 | -16).positive) | ||
| XCTAssertFalse((-17 | 42).positive) | ||
| XCTAssertFalse((1 | -111).positive) | ||
|
|
||
| XCTAssertTrue((0 | 1).positive) | ||
| XCTAssertTrue((-0 | -1).positive) | ||
| XCTAssertTrue((0 | -1).positive) | ||
| XCTAssertTrue((-0 | 1).positive) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Change these to > 0
| XCTAssertEqual( | ||
| (Unit.meter / Unit.second.pow(2.over(5))).symbol, | ||
| "m/s^(2|5)" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we get some fractional exponent parser tests in this MR as well? It looks like we've only added tests that verify that we can go from Swift -> String, but not from String -> Swift, and I think we haven't updated the parser to support fractional exponent equations: https://github.com/CrownedPhoenix/Units/blob/87a14f4d211172678d5f0c70c19a4a71b2d6599a/Sources/Units/Parser.swift#L166
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon second look, we probably don't need to update the parser - that code I linked only applies to exponents of numbers, not units. 🤦
Probably still good to add some tests of parsing fractional units though.
| if symbolChunks.count == 2 { | ||
| guard let expInt = Int(String(symbolChunks[1])) else { | ||
| guard let expInt = Fraction(String(symbolChunks[1])) else { | ||
| throw UnitError.invalidSymbol(message: "Symbol '^' must be followed by an integer: \(equation)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can we update this error message to specify that it has to be an integer or fraction?
| XCTAssertEqual( | ||
| (Unit.meter / Unit.second.pow(2.over(5))).symbol, | ||
| "m/s^(2|5)" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon second look, we probably don't need to update the parser - that code I linked only applies to exponents of numbers, not units. 🤦
Probably still good to add some tests of parsing fractional units though.
No description provided.