Rename Magnitude numerator/denominator traits #83
Labels
⬇️ affects: code (interfaces)
Affects the way end users will interact with the library
📁 kind: cleanup
Making the library nicer in some way, without affecting functionality much or at all
💪 effort: small
Milestone
Comments
chiphogg
added
📁 kind: bug
chiphogg
added
⬇️ affects: code (interfaces)
chiphogg
added a commit
that referenced
this issue
Jun 19, 2023
The current definitions of numerator and denominator assume we're talking about the integer part only. This policy is error-prone and unclear from the callsite. It also holds us back from automatically generating labels for `Magnitude<...>` types, which would improve basic library usability significantly at the margins. Of course, we need the current definitions, which is why we added them in the first place. To continue meeting our needs, we add a new trait operation: `integer_part(m)`. This is equivalent to the old behaviour of `numerator(m)`: it pulls out the _integer part_ of the numerator of `m`. But it frees up `numerator(m)` for a more natural interpretation. On the implementation side, I took the opportunity to replace a recursive implementation with one which is expanded inline, which may be slightly more efficient. This new approach exposed a bug in our parameter pack handling: apparently, the unary product only actually worked for single-element packs. I added a test to expose this and made the test pass. I also grepped for `numerator`, `NumeratorT`, `denominator`, and `DenominatorT`, to make sure we updated all of our old uses. This grepping caught a use case which we can't currently test for: rational exponents (#116). There _is_ a commented-out test case here, in the `sqrt.CanConvertIfConversionFactorRational` case from `//au:math_test`, which is commented out because it currently fails to compile. (Note that even if we had missed this update, it still wouldn't have compiled because of a safeguard in `magnitude.hh`.) In any case, I did find and update this use case, which will save a little work in case we ever attempt #116. Testing plan: - [x] New unit tests - [x] Grepped existing uses - [ ] Compile time impact measurements Fixes #83.
3 tasks
chiphogg
added a commit
that referenced
this issue
Jun 20, 2023
The current definitions of numerator and denominator assume we're talking about the integer part only. This policy is error-prone and unclear from the callsite. It also holds us back from automatically generating labels for `Magnitude<...>` types, which would improve basic library usability significantly at the margins. Of course, we need the current definitions, which is why we added them in the first place. To continue meeting our needs, we add a new trait operation: `integer_part(m)`. This is equivalent to the old behaviour of `numerator(m)`: it pulls out the _integer part_ of the numerator of `m`. But it frees up `numerator(m)` for a more natural interpretation. On the implementation side, I took the opportunity to replace a recursive implementation with one which is expanded inline, which may be slightly more efficient. This new approach exposed a bug in our parameter pack handling: apparently, the unary product only actually worked for single-element packs. I added a test to expose this and made the test pass. I also grepped for `numerator`, `NumeratorT`, `denominator`, and `DenominatorT`, to make sure we updated all of our old uses. This grepping caught a use case which we can't currently test for: rational exponents (#116). There _is_ a commented-out test case here, in the `sqrt.CanConvertIfConversionFactorRational` case from `//au:math_test`, which is commented out because it currently fails to compile. (Note that even if we had missed this update, it still wouldn't have compiled because of a safeguard in `magnitude.hh`.) In any case, I did find and update this use case, which will save a little work in case we ever attempt #116. Testing plan: - [x] New unit tests - [x] Grepped existing uses - [x] Compile time impact measurements Fixes #83.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
numerator(m)gives the integer part of the numerator. This is counterintuitive. It should instead correspond to "the parts that a human would put on top of the fraction". Similar reasoning applies todenominator(m).To fill the role played by the current versions of
numeratoranddenominator, we can either make "integer"-named versions, or a genericinteger_part(m)function which can compose with them.The text was updated successfully, but these errors were encountered: