-
Notifications
You must be signed in to change notification settings - Fork 26
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
Si matching #173
Si matching #173
Conversation
Codecov Report
@@ Coverage Diff @@
## main #173 +/- ##
==========================================
- Coverage 98.11% 97.46% -0.65%
==========================================
Files 9 9
Lines 4875 5012 +137
==========================================
+ Hits 4783 4885 +102
- Misses 92 127 +35 |
Co-authored-by: Philip Top <[email protected]> Co-authored-by: HELICS-bot <[email protected]>
Co-authored-by: Philip Top <[email protected]> Co-authored-by: HELICS-bot <[email protected]>
Co-authored-by: Philip Top <[email protected]> Co-authored-by: HELICS-bot <[email protected]>
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.
Looks very promising. 👍
EXPECT_EQ(to_string(unit_from_string("dm")), "dm"); | ||
|
||
EXPECT_EQ(to_string(unit_from_string("km")), "km"); | ||
EXPECT_EQ(to_string(unit_from_string("Km")), "km"); |
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.
Any particular reason for supporting uppercase K
here?
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.
Correct or not I have seen Kg
get used in a number of contexts for kilogram. I suppose I could just remove the support for general upper case 'K' here and specifically support Kg
units/units.hpp
Outdated
constexpr std::uint32_t cooking{2U}; | ||
constexpr std::uint32_t astronomy{4U}; | ||
constexpr std::uint32_t nuclear{8U}; | ||
constexpr std::uint32_t surveying{9U}; |
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.
So surveying is ucum
and nuclear
?
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.
guess I need to double check the bit sequences again
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 tweaked the actual codes a little more a little happier with them but I suspect more revisions will be necessary in future versions.
units/units.cpp
Outdated
case 'm': | ||
return 0.001; | ||
case 'k': | ||
case 'K': |
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.
Is this actually SI?
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.
removed the capital K from the strict SI for kilo.
Co-authored-by: Philip Top <[email protected]> Co-authored-by: HELICS-bot <[email protected]>
Co-authored-by: Philip Top <[email protected]> Co-authored-by: HELICS-bot <[email protected]>
add unit domains in an effort to better match SI standards for some specific units.
Add additional documentation
Address Issue #172