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

Scaling 100.0 does not recover all integral percentage values #275

Open
chiphogg opened this issue Apr 1, 2021 · 2 comments
Open

Scaling 100.0 does not recover all integral percentage values #275

chiphogg opened this issue Apr 1, 2021 · 2 comments
Milestone

Comments

@chiphogg
Copy link

chiphogg commented Apr 1, 2021

This test fails for i in 29, 57, 58.

TEST(PercentT, RecoversInputValues) {
    for (int i = 0; i <= 100; ++i) {
        EXPECT_EQ(i, static_cast<int>(units::concentration::percent_t{i}.to<double>() * 100.));
    }
}

I think this ultimately stems from the inconsistent handling of percent_t vs. other units types, as elaborated in #81.


Please include the following information in your issue:

  1. Which version of units you are using: v2.3.1
  2. Which compiler exhibited the problem (including compiler version): gcc 7.5.0
@nholthaus
Copy link
Owner

added a test for this to 3.x

TEST(Consistency, recovers_input_values)
{
	for (int i = 0; i <= 100; ++i) {
		EXPECT_DOUBLE_EQ(i, units::concentration::percent<double>(i).value() * 100);
	}
}

@chiphogg
Copy link
Author

Thanks for following up! But I don't think this test reproduces the problem. The goal of the original code was to recover the input value as an integer.

The static cast in the OP truncates. If by chance we end up slightly below, rather than slightly above, we're off by a whole 1: the consequences of small errors are asymmetrical.

I think this will always happen whenever anyone uses this strategy. It's a reasonable looking strategy at a glance, and might pass code review, but it'll just always be error prone.

The point of the original post (which was very much not obvious---sorry!) was that the real problem is that people are motivated to use this strategy, when what they want to do is to retrieve the original value, as if stored in a container.

It took me a long time to recognize this, but the notion of "value" is ambiguous specifically for dimensionless units other than 1. Consider 75%. By "value", in some contexts we might mean "the value of the quantity" (0.75), and in other contexts we might mean "the value stored" (75). This kind of dimensionless unit is the only kind where these notions are both different and not guarded by the type system.

In Aurora's library, I ended up deciding to forbid implicit conversions in both directions for this kind of unit. I don't know what solution is most appropriate for your library, but it might be nice to have some unambiguous way to say "retrieve the stored value in the given unit".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants