Skip to content

✨Add arithmetic operators for inversely related quantities #1586

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

angularsen
Copy link
Owner

Fixes #1566

Quantities with inverse relations, such as Length and ReciprocalLength, were missing arithmetic operators for things like Length * ReciprocalLength = double and ReciprocalLength / double = Length.

Changes

  • Add missing arithmetic for quantities with inverse relations
    • Affected: Density, ElectricConductivity, ElectricResistivity, Length, ReciprocalLength, Area, ReciprocalArea
    • Implement IMultiplyOperators<ReciprocalLength, Length, double> or similar
    • Add public static Length operator /(double value, ReciprocalLength reciprocalLength) or similar
    • Add public static double operator *(ReciprocalLength reciprocalLength, Length length) or similar
  • Change UnitRelations.json to use double instead of 1, and a new -- Inverse configuration flag
  • Add IsInverse and IsDerived properties to QuantityRelation
  • Add test ArithmeticOperators_Relational for each quantity
  • Add test InverseMethod for each quantity

Fixes #1566

Quantities with inverse relations, such as Length and ReciprocalLength, were missing arithmetic operators for things like `Length * ReciprocalLength = double` and `ReciprocalLength / double = Length`.

### Changes
- Add missing arithmetic for quantities with inverse relations
  - Affected: `Density`, `ElectricConductivity`, `ElectricResistivity`, `Length`, `ReciprocalLength`, `Area`, `ReciprocalArea`
  - Implement `IMultiplyOperators<ReciprocalLength, Length, double>` or similar
  - Add `public static Length operator /(double value, ReciprocalLength reciprocalLength)` or similar
  - Add `public static double operator *(ReciprocalLength reciprocalLength, Length length)` or similar
- Change `UnitRelations.json` to use `double` instead of `1`, and a new `-- Inverse` configuration flag
- Add `IsInverse` and `IsDerived` properties to `QuantityRelation`
- Add test `ArithmeticOperators_Relational` for each quantity
- Add test `InverseMethod` for each quantity
@angularsen angularsen changed the title ✨Add arithmetic operators for inverse/relational quantities ✨Add arithmetic operators for inversely related quantities Jul 26, 2025
@angularsen angularsen requested a review from lipchev July 26, 2025 23:59
Copy link

codecov bot commented Jul 27, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (master@0da4a5a). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff            @@
##             master   #1586   +/-   ##
========================================
  Coverage          ?     93%           
========================================
  Files             ?     314           
  Lines             ?   29098           
  Branches          ?       0           
========================================
  Hits              ?   27236           
  Misses            ?    1862           
  Partials          ?       0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lipchev
Copy link
Collaborator

lipchev commented Jul 27, 2025

Are you intentionally going about looking for ways to create more merge conflicts for my 🐲 PR? Why are we still investing in the double? Are the Inverse operators more important than reviewing and releasing the fix for the == and < operators? This has taken so long, I had even forgotten about the things I've done regarding the implicit relationships (such as the Inverse).

@angularsen
Copy link
Owner Author

Sorry, I realize this adds pain for you and probably duplicates some of your work. The big PR has been on the horizon for a very long time and has had way too much stuff in it and I've not spent enough time with it to have a good overview.

I'm just trying to close open tickets that have been cropping up for the past months now that I finally found a little time and energy, and I've tried to keep the changes small and targeted.

Why are we still investing in the double

Well, it's what we currently have in master. Please mind, you have way more insight into your upcoming changes than I have so I can't plan well for it at this point.

Is the big PR ready for review or are you still massaging it or trying to split off smaller PRs?

@lipchev
Copy link
Collaborator

lipchev commented Jul 27, 2025

Well, it's what we currently have in master. Please mind, you have way more insight into your upcoming changes than I have so I can't plan well for it at this point.

That's my point, the PR has been sitting there forever- and while I'm not saying you should have done a full line-by-line review by now, frankly it feels like you haven't even taken the time to run the samples.

Is the big PR ready for review or are you still massaging it or trying to split off smaller PRs?

I will continue extracting smaller PRs from it (as long as that's possible), but at some point- you'd have to give me an answer about the overall direction : are we moving towards removing the double (in v6) or are we just doing optimizations to it.

@angularsen
Copy link
Owner Author

That's fair, I haven't run any of your samples or even attempted reviewing the PR yet as my understanding was that you were still working on it and the chunks you split off were useful on their own so my focus were on those.

I'll focus on reviewing the overall idea and design of the PR next then, and the samples you provided, to help determine if the change feels right for the library and for v6.

@lipchev
Copy link
Collaborator

lipchev commented Jul 27, 2025

FYI, I'm just wrapping up with a PR for the changes to the IQuantity interface (the next task on the list)- in the corresponding interfaces of the 🐲 PR there are still some bits which are either commented-out or under #if EXTENDED_STUFF tag), I'll remove them as soon as we review the changes in the smaller PR (coming in the next 15m or so).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] more operators about Reciprocal units.
2 participants