-
Notifications
You must be signed in to change notification settings - Fork 15
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
make JSON files smaller by limiting the precision #499
Conversation
Codecov Report
@@ Coverage Diff @@
## main-dev #499 +/- ##
============================================
+ Coverage 76.64% 76.65% +0.01%
============================================
Files 97 97
Lines 17357 17365 +8
============================================
+ Hits 13304 13312 +8
Misses 4053 4053
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
please conside the following changes
- use
np.round
instead ofround
- use comprehensions instead if create/modify loops
- add a test tat covers this new test that asserts the
round_floats
functionalityassert round_floats(16.055, 2) == 16.055 assert round_floats([16.055], 2) == [16.055] assert round_floats({"value":16.055}, 2) == {"value":16.055}
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.
This is what I mean regarding the usage of comprehensions instead of create/modify loops
if isinstance(in_data, (float, np.float)):
return np.round(in_data, precision)
if isinstance(in_data, (list, tuple)):
return [round_floats(v, precision=precision) for v in in_data]
if isinstance(in_data, dict):
return {k:round_floats(v, precision=precision) for k, v in in_data.items()}
return in_data # or raise a ValueError
I never meant to say that the if isinstance
logic should be folded into the comprehension
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.
Thanks for the changes.
Could you also ads some tests that assert on the desired functionality.
assert round_floats(16.055, 2) == 16.06
assert round_floats([16.055], 2) == [16.06]
assert round_floats({"value":16.055}, 2) == {"value":16.06}
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.
👍 ship it
Will merge as soon as CI is finished |
attempt to fix #366