-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Model summary: add 1 decimal place #4745
Conversation
mind adding a test for this? |
The docs are the tests :) but I can add more tests if the change is accepted. |
@jonashaag the doctest is sufficient, you just need to fix the decimal point in the other places where the model summary string is tested. |
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.
it is a good point to show decimal when there is only 1 digit but later when you have thousands the meaning of decimal is meaningless >> 5,000.0
I can add a special case for T if you want |
I would say whatever is above 100 does not need decimal and lower than 100 shows one decimal |
Show 1999 parameters as 1.9 K and 1000 parameters as 1.0 K, rather than both as 1 K.
OK, please review again. |
Codecov Report
@@ Coverage Diff @@
## master #4745 +/- ##
======================================
Coverage 93% 93%
======================================
Files 117 117
Lines 8972 8974 +2
======================================
+ Hits 8345 8347 +2
Misses 627 627 |
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.
nice, thx :]
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.
Great addition ! Would you mind adding a test ?
As said, the tests are in the docs (doctest) but if you want I can add separate unit tests. |
Show 1999 parameters as 1.9 K and 1000 parameters as 1.0 K, rather than both as 1 K. Co-authored-by: chaton <thomas@grid.ai> Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> Co-authored-by: Sean Naren <sean.narenthiran@gmail.com>
Show 1999 parameters as 1.9 K and 1000 parameters as 1.0 K, rather than both as 1 K. Co-authored-by: chaton <thomas@grid.ai> Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> Co-authored-by: Sean Naren <sean.narenthiran@gmail.com> (cherry picked from commit 8dfbf63)
What does this PR do?
Show 1999 parameters as 1.9 K and 1000 parameters as 1.0 K, rather than both as 1 K.
Before submitting
PR review
Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In in short, see following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃