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.

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

Rjf/279 simplify unit configurations and conversions #281

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Rjf/279 simplify unit configurations and conversions #281

wants to merge 24 commits into from

Conversation

robfitzgerald
Copy link
Collaborator

@robfitzgerald robfitzgerald commented Feb 5, 2025 •
edited
Loading

This PR begins the work described in this milestone by introducing a Convert trait for units. changes in this PR are due to that change and the discovery + design task for the scope of this milestone.

along the way:

  • SpeedUnit is now a tuple newtype struct of Distance and Time units
    • conversion of SpeedUnits leans on the underlying distance + time conversion factors
  • EnergyRateUnit is now an enum, each variant a tuple newtype struct of Energy and Distance units
    • comes in distance/energy and energy/distance variants to support convention (mpg) vs our preference during routing (gpm)
  • Convert methods take a Cow<U> for the given unit type. this allows us to avoid unnecessary copying when the conversion is between two of the same unit type (such as kwh -> kwh)
  • FromStr, Display, Serialize, and Deserialize ops expanded to allow for some more human-legible configurations, like using "kph" or "kilometers/hour" for a speed unit value, or "km" for kilometer, or "kWh" for kilowatt hours
  • added a few tests for these new capabilities (deserialize + convert)

Closes #279.

@robfitzgerald robfitzgerald mentioned this pull request Feb 6, 2025
Open
@robfitzgerald robfitzgerald added this to the Sprint: Units fixes milestone Feb 6, 2025
@robfitzgerald
Copy link
Collaborator Author

i kept running into confusion as to whether an energy rate unit should follow the well-known convention of distance/energy unit, or if we should impose energy/distance unit, since that is more efficient for routing. but now i'm thinking, we should support both, and call me crazy, but i've started making that change:

pub enum EnergyRateUnit {
    DistancePerEnergy(DistanceUnit, EnergyUnit),
    EnergyPerDistance(EnergyUnit, DistanceUnit),
}

internally we can convert things to EnergyRateUnit::EnergyPerDistance(KilowattHours, Meters). this means i need to add a Convert implementation for EnergyRateUnit.

@robfitzgerald robfitzgerald marked this pull request as ready for review February 24, 2025 22:37
@robfitzgerald
Copy link
Collaborator Author

@nreinicke ok, now ready for review. also added another issue for volume in energy units which would further clean things up (see #293, maybe a good first issue?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nreinicke nreinicke Awaiting requested review from nreinicke

@jakeholden jakeholden Awaiting requested review from jakeholden

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Sprint: Units fixes
Development

Successfully merging this pull request may close these issues.

Update traversal model unit representation
1 participant
@robfitzgerald