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

Changing the Dimensions and BaseUnit for the FuelEfficiency #1489

Merged
merged 6 commits into from
Jan 19, 2025

Conversation

lipchev
Copy link
Collaborator

@lipchev lipchev commented Jan 3, 2025

Changing the Dimensions and BaseUnit for the FuelEfficiency:

  • BaseUnit changed from LiterPer100Kilometers (defaults to +Infinity) to KilometersPerLiter (defaults to 0)
  • added Dimensions of "L": -2 (was previously treated as a dimensionless quantity)
  • updated the relevant tests
  • skipped all SI-related tests

- BaseUnit changed from LiterPer100Kilometers (defaults to +Infinity) to KilometersPerLiter (defaults to 0)
- added the MeterPerCubicMeter (m/m³) unit
- updated the relevant tests
@lipchev
Copy link
Collaborator Author

lipchev commented Jan 3, 2025

I was also going to include this as part of the UnitRelations.json:

"Length.Kilometer = FuelEfficiency.KilometerPerLiter * Volume.Liter -- NoInferredDivision",

but ended up reverting the change (not that it didn't work or anything). Here is how this would affect the two quantities:

    public readonly partial struct FuelEfficiency :
        IArithmeticQuantity<FuelEfficiency, FuelEfficiencyUnit>,
#if NET7_0_OR_GREATER
        IMultiplyOperators<FuelEfficiency, Volume, Length>,
#endif
    public readonly partial struct Volume :
        IArithmeticQuantity<Volume, VolumeUnit>,
#if NET7_0_OR_GREATER
        IMultiplyOperators<Volume, Molarity, AmountOfSubstance>,
        IMultiplyOperators<Volume, ReciprocalLength, Area>,
        IDivisionOperators<Volume, Length, Area>,
        IMultiplyOperators<Volume, Length, AreaMomentOfInertia>,
        IDivisionOperators<Volume, VolumeFlow, Duration>,
        IMultiplyOperators<Volume, EnergyDensity, Energy>,
        IMultiplyOperators<Volume, FuelEfficiency, Length>,
        IMultiplyOperators<Volume, ReciprocalArea, Length>,
        IDivisionOperators<Volume, Area, Length>,
        IMultiplyOperators<Volume, Density, Mass>,
        IMultiplyOperators<Volume, MassConcentration, Mass>,
        IDivisionOperators<Volume, SpecificVolume, Mass>,
        IDivisionOperators<Volume, Mass, SpecificVolume>,
        IDivisionOperators<Volume, Duration, VolumeFlow>,
#endif

Here are the pros and cons to consider:

  • the multiplication operator seems reasonable: we multiply some amount (volume) of fuel by some fuel efficiency and get the travel distance. Technically there isn't anything wrong with dividing some length by a given fuel efficiency, in order to get the amount of fuel- however I don't immediately think of fuel efficiency as the result of dividing a length by volume (although I can't say I have any particular default quantity in mind 🤷 ).
  • the downside (even with the NoInferredDivision) is that it binds the two quantities together, making it harder (if not impossible) to separate the Volume and the FuelEfficiency later (e.g. UnitsNet.Common + UnitsNet.Extended)
  • as a middle ground, we could just add (by hand) the multiply and divide operators on the side of the FuelEfficiency, however this might seem odd w.r.t. the multiplication..

@angularsen A lot of important decisions for you to make in this PR 😄

PS Of course we could just close this PR: I don't care much either way, I had the json file changed while testing other stuff (from months ago), so one way or another I wanted to resolve my change-set..

@lipchev
Copy link
Collaborator Author

lipchev commented Jan 19, 2025

@angularsen Ok, I've removed the MeterPerCubicMeter , skipped the tests and updated the PR summary to reflect the changes.

I've also updated the summary of the related issue: #1463, have a look at the remaining tasks..

@angularsen angularsen merged commit fa38b43 into angularsen:release/v6 Jan 19, 2025
1 check passed
@angularsen
Copy link
Owner

the downside (even with the NoInferredDivision) is that it binds the two quantities together, making it harder (if not impossible) to separate the Volume and the FuelEfficiency later (e.g. UnitsNet.Common + UnitsNet.Extended)

I'm not sure I follow, how is this worse than binding to other quantities? Is it because you consider this a less-widely-used quantity?

Regarding extensibility and splitting up into packages, this is tricky with static typed stuff that we primarily deal with, but I have some ideas in #1500

as a middle ground, we could just add (by hand) the multiply and divide operators on the side of the FuelEfficiency, however this might seem odd w.r.t. the multiplication..

If the inferred operators don't make sense, then yes, we always have the option of manually adding them.

@lipchev
Copy link
Collaborator Author

lipchev commented Jan 20, 2025

the downside (even with the NoInferredDivision) is that it binds the two quantities together, making it harder (if not impossible) to separate the Volume and the FuelEfficiency later (e.g. UnitsNet.Common + UnitsNet.Extended)

I'm not sure I follow, how is this worse than binding to other quantities? Is it because you consider this a less-widely-used quantity?

Yes, I'd say that the minimal quantity set (e.g. UnitsNet.Common) would have to include the SI quantities + all of their associated operators (e.g. Mass -> Density) + all of their associations (e.g. Density -> KinematicViscosity, DynamicViscosity etc..).. I haven't tested how many quantities would get covered by this graph, but If there are enough left-overs we could put them in a separate nuget.

@angularsen
Copy link
Owner

Alright, I fear it will be tough to split up into two packages where one cannot depend on the other, but maye there is such a cut.

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.

2 participants