-
Notifications
You must be signed in to change notification settings - Fork 87
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
implement value_cast<ToQ> and value_cast<ToQP> #571
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
c51baae
value_cast overloads for ToQ and ToQP
burnpanck c0efdb1
better handle simultaneous change of representation, unit and point_o…
burnpanck f30fac1
added a bit of documentation
burnpanck 5412e27
Merge branch 'master' into feature/more-value-casts
burnpanck ae912c0
added one more test; highlighting an issue with detail::common_magnit…
burnpanck 1e287c2
fixed value_cast<ToQP> with matching units but differing point_origin
burnpanck 2459409
created sudo_cast<QP> overload, and merged shared computation into se…
burnpanck 4b809ec
Merge branch 'master' into feature/more-value-casts
burnpanck 7589ba6
Merge branch 'master' into feature/more-value-casts
burnpanck da17b01
added a bit more detail to the documentation of the quantity-point ov…
burnpanck b113f6a
Merge branch 'master' into feature/more-value-casts
mpusz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are assuming here an integral representation type, which often will not be the case as
double
is the default in the library. In cases of a floating point representation type, we should calculate the entire conversion factor in one step and then do only one multiplication operation on the value to get the result. Otherwise, we will not generate assembly equivalent to operations ondouble
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If my previous comment was unclear, I will just note that the
relative_point_origin
stores an offset, which has its own unit and representation as well. We could factor in this conversion here as well (instead of callingpoint_for()
as a separate step).However, maybe it would be too complex to do it here anyway and the measurable performance gains would not be that big.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, those considerations about order of the conversion operations are most relevant for integral types, where both underflow and overflow are likely. It doesn't hurt for double either. However, I'm not sure what you mean by "calculate the entire conversion factor in one step". A change of point-origin requires an addition. A change in unit requires a multiplication. This implementation here does at most one multiplication and at most one addition in all cases. Here is how:
sudo_cast<Q>
implementation to select the "best" way to do that multiplication, and avoids the additionsudo_cast
(no-op in the case of floatingpoints). previously, that was avalue_cast
, but I figured that within asudo_cast
,value_cast
is off limit..point_for
; hopefully, the implementation of.point_for
is such that this is a single addition and no multiplicationsudo_cast
.sudo_cast
. If the intermediate representation type is the same as the source erpresentation and the units remain the same, this is ano_op
..point_for
sudo_cast
. No-op for floating-point types.So I believe this implementation is as efficient as it can be through a careful selection of types. While both code-paths invoke
sudo_cast
twice, there is always one where the source and target units match and no multiplication is needed. For floating-point, there is also not cast, so that cast is a no-op. Furthermore, the implementation here does never do any arithmetic itself - it delegates the addition to.point_for
and the multiplication tosudo_cast<Q>
. Personally, I would even prefer to callvalue_cast
instead, because in each of the two code-paths, one of the casts is a pure representation cast, which would be more clear asvalue_cast<rep>
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, now I understand you are indicating that
.point_for
may potentially change the unit and representation type? In that case, you are of course right, we should replace the.point_for
call here with a dedicated implementation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean something like this:
Precalculation of
ratio
as aconstexpr
variable ensures that the assembly will include only one multiplication by this constant. Theelse
branch typically results in more assembly instructions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@burnpanck, are you going to work on this here? Or should we rebase, merge, and then refactor in the next PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, work interfered. Yeah, I intend to do a replacement of
point_for
by a direct implementation which avoids all further unit conversions. However, I believe the code as-is would be mergeable too. As mentioned before, there should definitely be no regressions in this PR as of now. All unit conversions are as efficient as they can be (they re-use the existing implementation). The currentvalue_cast<QP>
is no worse than what you would reasonably implement using the tools given by this library as a user (namely, usingpoint_for
). The converting constructor ofquantity_point
is untouched and slightly worse than thisvalue_cast<QP>
concerning overflow or truncation with integer types. Maybe it would make sense to switch the converting constructor to delegate tovalue_cast<QP>
and then merge. We can then postpone performance-improvement ofvalue_cast<QP>
to another PR, and perhaps discuss separately when the current converting constructor ofquantity_cast
should beexplicit
or not.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so let's do it this way. I need a few more days to finish the framework cleanup I am doing right now. It there will be no further changes in this PR when I am done, I will merge it as is and provide a new mp-units 2.2 release. After that we will continue this effort in the following PRs targeting the next release. If you will have some time in the upcoming days to work on it and you care for all of the changes to be provided in mp-units 2.2., please feel encouraged to do so.