-
-
Notifications
You must be signed in to change notification settings - Fork 114
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
Fix: reformat long seqs #806
base: master
Are you sure you want to change the base?
Conversation
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.
@joko3ono thanks for looking into this. Could you please add an explanation of what was the issue and how your change fixes the issue to the commit message, and also add some jest unit tests to demonstrate how it works + make sure we don't break it in future?
It's a bit hard to review the changes without context and learning what the bug is from scratch would be very time consuming as I'd have to come up with multiple test scenarios and execute them 😬
things look almost ok however, there needs to be correct rounding up of the final sequence length. See below that:
However...If we want to show a greater level of precision then we can show to at least 1 decimal place for any value that has units kbp, Mbp and Gbp. ie
divide these numbers by 1000000 and round to the nearest decimal place
|
@joko3ono also please make sure you add tests for the rounding so that it's not broken in future |
e8d0b07
to
103b934
Compare
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 @joko3ono, the code looks good now.
@yannickwurm do you want to test this or happy to merge?
3d9b939
to
564e9e6
Compare
564e9e6
to
2c2e387
Compare
resolve wurmlab/sequenceserver-cloud#415