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

Strange choice of units in to_string #202

Closed
jl-wynen opened this issue Jan 31, 2022 · 6 comments · Fixed by #213
Closed

Strange choice of units in to_string #202

jl-wynen opened this issue Jan 31, 2022 · 6 comments · Fixed by #213

Comments

@jl-wynen
Copy link
Contributor

to_string produces strange results for some units. E.g.

to_string(units::unit_from_string("us / angstrom^2"))  // -> 9290304000000s/ft^2
to_string(units::unit_from_string("us / angstrom"))  // -> 10N/mW

Neither of the two cases is particularly readable.

It would be great if the output would be closer to the input. I realise that it is not always so clear what the output should be, especially if the unit is the result of a long computation. But is there a way to improve the current situation?

@phlptp
Copy link
Collaborator

phlptp commented Jan 31, 2022

I am sure there are many ways to improve it, mostly a matter of which way to improve it. It is kind of a problem of infinite number of strings to generate a particular unit, then infinite number of ways to generate a string from that unit.

The trick is figuring out which way is optimum in some sense, and to do it in some way in somewhat efficient maintainable code.

My general thought I suppose is that if you have a way to make better output in the to_string in a straightforward fashion I am very happy to take a PR or listen to specific suggestions, otherwise for specific applications with a few anomalies, the code does support custom units which can be used to generate specific strings for specific units that might come up in a particular application more regularly but the default output is not ideal.

@jl-wynen
Copy link
Contributor Author

jl-wynen commented Feb 1, 2022

I think custom units would make things too complicated for us because we have to handle units in a generic manner.

Can you point me at the relevant piece of code? Is there a description of the algorithm somewhere?
I can't promise that I can find a better way, but I am happy to take a look when I find the time.

@phlptp
Copy link
Collaborator

phlptp commented Feb 1, 2022

I wish there was a single algorithm, but the reality is that the to_string operation is more a collection of heuristics to break down the unit and piece together a string that produces the unit exactly if it were used again in unit_from_string.

For example Line 330 in units.cpp defines an array that is looped through to test out various combinations of those units which are commonly used in combination with others. So if you dealt a lot with angstroms for example. I could see just adding a line to that array for angstroms, that would probably fix your above example. Obviously can't do that for every case, but as a 1 off might make sense.

At some point I want to add a way to add some user defined units into that loop for string generation but not sure when I will get to that.

Also what I meant for custom units is just adding a call to user defined units. This works well if there are just a couple strings or particular units you want the string formulated in a particular way. Not ideal if there are hundreds, but less than 5 probably makes sense. Then that particular unit will bypass all the other heuristics for string generation and just produce that string, all others would stay the same.

@phlptp
Copy link
Collaborator

phlptp commented Mar 25, 2022

I added a test and capability

 addUserDefinedUnit("angstrom", precise::distance::angstrom);

    auto str = to_string(units::unit_from_string("us / angstrom^2"));
    EXPECT_EQ(str, "us/angstrom^2");
    str = to_string(units::unit_from_string("us / angstrom"));
    EXPECT_EQ(str, "us/angstrom");
    clearUserDefinedUnits();

Basically if you have a specific unit you want prioritized in string generation you can add it to addUserDefinedUnit

@jl-wynen
Copy link
Contributor Author

Sounds good, thanks!

This made me think, would it make sense to allow users to override / modify the default testUnits? We discourage use of, among others, imperial units. So it might be nice to remove ft, and Mi from the formatting options.
That would require making testUnits non-constexpr. Would that have a significant impact on performance?

@phlptp
Copy link
Collaborator

phlptp commented Mar 25, 2022

I recently shifted the unit string definitions to a constexpr array. Previously it was getting constructed on a stack to make a const unordered_map. So the map itself isn't constexpr so I doubt changing it around would impact performance much.

With the constexpr array used to generate the map it still gets constructed into a const map, but the stack usage is much lower. The map itself it not constexpr, just const. Now that it is split out into a separate file (unit_conversion_maps.hpp) I have toyed with the idea of later on splitting that into a SI compliant part and all others. So then it could be a compile time switch(or possibly runtime options) to make it strictly SI compliant with no additional code or user control. The next step beyond that might be to make the map usage itself a pointer, and allow user modification. From some of the feedback having a pure SI mode and "all units" mode would cover a majority of user cases. The other option I am considering is a also loading the const map from a file in some way to maybe have a chance to support other languages in a little better fashion. But that could get a bit circular.

Anyway there is a bit of evolving of the library that will happen as I get time.

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 a pull request may close this issue.

2 participants