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

Ensure value_cast<QP> uses the minimum number of operataions needed #598

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

burnpanck
Copy link
Contributor

This PR is supposed to continue from the discussion here, where value_cast<QP> was introduced.

Basically, value_cast<QP> does three steps:

  1. Convert the representation/unit of the input quantity_point to an intermediate representation, not touching the origin
  2. Change the origin by adding a suitable offset
  3. Convert the representation/unit of the intermediate quantity_point to the final representation.

However, in the current implementation, the second step (change of origin) may in fact introduce more representation conversions internally: to a common representation between the offset and the value. However, here, that is not necessary, because the representation of the value is already chosen suitably.

@mpusz mpusz added this to the v2.3.0 milestone Jul 20, 2024
@mpusz
Copy link
Owner

mpusz commented Jul 29, 2024

@burnpanck, I fixed all the CI issues today. If you could rebase to master, we would see this change's real CI status.

@mpusz
Copy link
Owner

mpusz commented Sep 6, 2024

@burnpanck, do you still plan to work on your PRs?

@mpusz
Copy link
Owner

mpusz commented Sep 13, 2024

Hi @burnpanck! Do you plan to work on those in the near future. I plan to release mp-units 2.3 soon.

@burnpanck burnpanck force-pushed the feature/efficient-value-cast-QP branch from 341b4eb to 61198fe Compare September 14, 2024 16:16
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