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

Fix units::modf() for scaled quantities with a frac part #312

Open
wants to merge 1 commit into
base: v3.x
Choose a base branch
from

Conversation

ts826848
Copy link
Contributor

units::modf() appears to add a spurious scaling factor to the fractional result it returns. For example, this test code:

percent<>      pval{202.5};
double         pmodfr1;
decltype(pval) pmodfr2;
EXPECT_EQ(std::modf(pval.to<double>(), &pmodfr1), units::modf(pval, &pmodfr2));
EXPECT_EQ(pmodfr1, pmodfr2);

Fails with this message:

units/unitTests/main.cpp:4649: Failure
Expected equality of these values:
  std::modf(pval.to<double>(), &pmodfr1)
    Which is: 0.025
  units::modf(pval, &pmodfr2)
    Which is: 0.00025
[  FAILED  ] UnitMath.modf (0 ms)

Not 100% confident I understand the specifics of the C++ rules here, but I think this line from units::modf() is the culprit:

dimensionlessUnit fracpart = std::modf(x.template to<decltype(intp)>(), &intp);

I think this uses uses copy-initialization to initialize fracpart by using the implicit converting constructor for linear dimensionless units to create a dimensionlessUnit from the std::modf() result, then using that to direct-initialize fracpart.

The problem is that because this uses the implicit converting constructor for linear dimensionless units, the scaling done to produce a value to pass to std::modf is not "undone" when reconstituting fracpart. Instead, the std::modf() result is taken as-is to produce a dimensionlessUnit value, effectively resulting in the scaling factor applying twice by the time the units::modf() result is turned back into a double.

For example, passing percent<>{202.5} to units::modf() results in 2.025 being passed to std::modf(), which returns 0.025. This is passed to the converting constructor, resulting in percent<>{0.025} (!), which is then used to direct-initialize fracpart, which is then returned. This is then turned into a double in EXPECT_EQ(), applying the percent<> scaling factor once again to get a final value of 0.00025.

This can be fixed by first making a dimensionless<> from the std::modf() result, which results in the unit converting constructor being called, which ensures the scaling factor is reapplied when initializing fracpart.

intpart is not affected by this issue because operator=() more or less already does this by making a dimensionless<> from its argument before changing *this.

units::modf() appears to add a spurious scaling factor to the fractional
result it returns. For example, this test code:

    percent<>      pval{202.5};
    double         pmodfr1;
    decltype(pval) pmodfr2;
    EXPECT_EQ(std::modf(pval.to<double>(), &pmodfr1), units::modf(pval, &pmodfr2));
    EXPECT_EQ(pmodfr1, pmodfr2);

Fails with this message:

    units/unitTests/main.cpp:4649: Failure
    Expected equality of these values:
      std::modf(pval.to<double>(), &pmodfr1)
        Which is: 0.025
      units::modf(pval, &pmodfr2)
        Which is: 0.00025
    [  FAILED  ] UnitMath.modf (0 ms)

Not 100% confident I understand the specifics of the C++ rules here, but
I think this line from units::modf() is the culprit:

    dimensionlessUnit fracpart = std::modf(x.template to<decltype(intp)>(), &intp);

I think this uses uses copy-initialization to initialize fracpart by
using the implicit converting constructor for linear dimensionless units
to create a dimensionlessUnit from the std::modf() result, then using
that to direct-initialize fracpart.

The problem is that because this uses the implicit converting
constructor for linear dimensionless units, the scaling done to produce
a value to pass to std::modf is not "undone" when reconstituting
fracpart. Instead, the std::modf() result is taken as-is to produce a
dimensionlessUnit value, effectively resulting in the scaling factor
applying twice by the time the units::modf() result is turned back into
a double.

For example, passing percent<>{202.5} to units::modf() results in 2.025
being passed to std::modf(), which returns 0.025. This is passed to the
converting constructor, resulting in percent<>{0.025} (!), which is then
used to direct-initialize fracpart, which is then returned. This is then
turned into a double in EXPECT_EQ(), applying the percent<> scaling
factor once again to get a final value of 0.00025.

This can be fixed by first making a dimensionless<> from the std::modf()
result, which results in the unit converting constructor being called,
which ensures the scaling factor is reapplied when initializing
fracpart.

intpart is not affected by this issue because operator=() more or less
already does this by making a dimensionless<> from its argument before
changing *this.
@ts826848
Copy link
Contributor Author

ts826848 commented Mar 27, 2023

I suppose I have a related question - the unit constructor for built-in types and operator=() for built-in types behave differently, since the former uses the provided value as-is while the latter first converts it to a dimensionless<> before changing *this. For example, this test code:

percent<> p1{5.0};
percent<> p2;
p2 = 5.0;
EXPECT_EQ(p1, p2);

Fails with this message:

Expected equality of these values:
  p1
    Which is: 5 pct
  p2
    Which is: 500 pct

Is this difference intended?

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.

1 participant