Skip to content

Inferred division from defined multiplication relations #1354

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Mar 1, 2024
Merged

Inferred division from defined multiplication relations #1354

merged 5 commits into from
Mar 1, 2024

Conversation

Muximize
Copy link
Contributor

@Muximize Muximize commented Jan 7, 2024 •
edited
Loading

In #1329 this proposal came up:

Another idea: generate division operators based on multiplication. Right now we define:

ElectricPotential.Volt = ElectricCurrent.Ampere * ElectricResistance.Ohm (and generate the reverse)
ElectricCurrent.Ampere = ElectricPotential.Volt / ElectricResistance.Ohm
ElectricResistance.Ohm = ElectricPotential.Volt / ElectricCurrent.Ampere

But those last two could also be generated based on the first.

This PR is an experiment implementing this.

Breaking changes:

  • TimeSpan = Volume / VolumeFlow => Duration = Volume / VolumeFlow

@Muximize Muximize mentioned this pull request Jan 7, 2024
Merged
@Muximize
Copy link
Contributor Author

Muximize commented Jan 25, 2024 •
edited
Loading

I rebased this on the latest changes in #1329 which makes it much cleaner.

Example of an interesting situation that occurs here, we previously defined:

MassFlow.GramPerSecond = Area.SquareMeter * MassFlux.GramPerSecondPerSquareMeter
MassFlux.KilogramPerSecondPerSquareMeter = MassFlow.KilogramPerSecond / Area.SquareMeter

If we infer division from multiplication, one of those has to add or lose a kilo 😅

@Muximize Muximize changed the base branch from master to release/v6 February 4, 2024 19:54
@angularsen
Copy link
Owner

Will review this next, hopefully in the next few days.

If we infer division from multiplication, one of those has to add or lose a kilo 😅

Haha 😆 But it will still work, right?
The inferred division will just use grams instead?

@Muximize
Copy link
Contributor Author

Muximize commented Feb 5, 2024

Yes, it will work, except we're not sure why the original contributor chose a particular unit for an operator and if it matters, with precision for example.

Also, this PR is more of an experiment than a serious proposal. There's some questions about the "magic" of it, and I took some liberties with implicit/explicit conversion between Density and MassConcentration that I'm not quite sure about. Maybe @lipchev can chime in on this?

@lipchev
Copy link
Collaborator

lipchev commented Feb 5, 2024

Yes, it will work, except we're not sure why the original contributor chose a particular unit for an operator and if it matters, with precision for example.

Also, this PR is more of an experiment than a serious proposal. There's some questions about the "magic" of it, and I took some liberties with implicit/explicit conversion between Density and MassConcentration that I'm not quite sure about. Maybe @lipchev can chime in on this?

Although the Density and Mass Concentration share the same units, they are not interchangeable- they describe different types of ratios. Casting between them (either implicitly or explicitly) makes no sense..

@Muximize
Copy link
Contributor Author

Muximize commented Feb 6, 2024

Thanks for the explanation! That makes the concept of inferred division a bit harder, given that we currently have:

Mass.Kilogram = Density.KilogramPerCubicMeter * Volume.CubicMeter
Mass.Kilogram = MassConcentration.KilogramPerCubicMeter * Volume.CubicMeter

which leads to these inferred ambiguous operators, which is what I tried to solve with my casting shenanigans:

Density.KilogramPerCubicMeter           = Mass.Kilogram / Volume.CubicMeter
MassConcentration.KilogramPerCubicMeter = Mass.Kilogram / Volume.CubicMeter

The latter is currently not defined, so skipping it would not be a breaking change. I've reworked this PR to do that and removed the casting stuff.

angularsen
angularsen requested changes Feb 17, 2024
View reviewed changes
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 like where this is going and would like to get it merged.
Some comments so far.

@Muximize Muximize mentioned this pull request Feb 19, 2024
Merged
@Muximize Muximize marked this pull request as ready for review February 24, 2024 12:56
@Muximize Muximize force-pushed the inferred-division branch 2 times, most recently from 44bd139 to 1950b51 Compare February 24, 2024 13:08
angularsen added a commit that referenced this pull request Feb 26, 2024
@Muximize @angularsen
Implicit cast between Duration and TimeSpan (#1365)
240b19d 
### Changes
- Change `Duration` from explicit to implicit cast to/from `TimeSpan`
- Remove operator overloads for `TimeSpan` now covered by implicit cast for all but left operands

### Background
See #1354 (comment)

One issue is that the operator overloads only work when `TimeSpan` is the right operand.

I changed the code generation to take this into account, but another option would be to make a breaking change where we just don't support `TimeSpan` as the left operand at all.

Then users would have to cast explicitly, or for multiplication just reverse the operands.

This would affect 13 operators:

```
TimeSpan * Acceleration
TimeSpan * ElectricCurrent
TimeSpan * ElectricCurrentGradient
TimeSpan * ForceChangeRate
TimeSpan * KinematicViscosity
TimeSpan * MassFlow
TimeSpan * MolarFlow
TimeSpan * Power
TimeSpan * PressureChangeRate
TimeSpan * RotationalSpeed
TimeSpan * Speed
TimeSpan * TemperatureChangeRate
TimeSpan * VolumeFlow
```

Of which only 6 are used in tests so I assume are supported in v5:
```
TimeSpan * KinematicViscosity
TimeSpan * MassFlow
TimeSpan * Power
TimeSpan * RotationalSpeed
TimeSpan * TemperatureChangeRate
TimeSpan * Speed
```

---------

Co-authored-by: Andreas Gullberg Larsen <andreas.larsen84@gmail.com>
angularsen
angularsen approved these changes Mar 1, 2024
View reviewed changes
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.

Looks good to me, just one minor comment we can address later.

@angularsen angularsen merged commit f7ce00b into angularsen:release/v6 Mar 1, 2024
angularsen added a commit to Tim-Borcherding/UnitsNet that referenced this pull request Mar 1, 2024
@angularsen
Remove redundant
649a56a 
Division now inferred by f7ce00b - Inferred division from defined multiplication relations (angularsen#1354)
@angularsen angularsen mentioned this pull request Mar 1, 2024
Merged
@Muximize Muximize deleted the inferred-division branch March 2, 2024 17:48
@angularsen
Copy link
Owner

Nuget on the way

Release UnitsNet/6.0.0-pre003 · angularsen/UnitsNet

@Muximize Muximize mentioned this pull request Jul 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@angularsen angularsen angularsen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Muximize @angularsen @lipchev