Skip to content

fix #2951 Use a thin space to indicate groups of 1000s in html/cli print out #3167

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hossam26644
Copy link
Contributor

@hossam26644 hossam26644 commented May 19, 2025

Implemented a format_number method in util that accepts a string or a number and returns the number separated by a thin space. Used the format_number for the string representation and the html output of trees, treesequence, and variants

Fixes #2951

Copy link

codecov bot commented May 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.76%. Comparing base (0663aaa) to head (9f4f8b6).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3167      +/-   ##
==========================================
- Coverage   89.61%   86.76%   -2.86%     
==========================================
  Files          28       11      -17     
  Lines       31939    24427    -7512     
  Branches     5866     4615    -1251     
==========================================
- Hits        28622    21193    -7429     
+ Misses       1886     1853      -33     
+ Partials     1431     1381      -50     
Flag Coverage Δ
c-tests 86.66% <ø> (ø)
lwt-tests 80.38% <ø> (ø)
python-c-tests 88.18% <ø> (ø)
python-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

see 17 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I guess the only downside of this is that people copying these numbers from the text won't get quite what they want. But, it that's just on pretty printing, that's OK?

Copy link
Member

@benjeffery benjeffery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just needs a fix for the tests. (I added a "fixes" tag to the PR description as it auto-closes the issue on merge)

Comment on lines 352 to 356
f"{util.format_number(counts[k])} ({
util.format_number(freqs[k] * 100, 2)}%)",
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This split is causing the module to fail to import.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I also had to change the regex of test_highlevel.py::TestTree::test_str to match the new format; please check that also @benjeffery

@hossam26644 hossam26644 force-pushed the print-out-split-large-numbers branch from f4e000d to 9b3b89c Compare May 20, 2025 13:06
@benjeffery
Copy link
Member

Looks good on HTML
Screenshot from 2025-05-20 16-54-25

I'm not sure about the CLI though, as in fixed-width font this looks wrong, and a comma would be better.
Screenshot from 2025-05-20 16-52-42

@hossam26644
Copy link
Contributor Author

hossam26644 commented May 20, 2025

I can change the separator to a comma for the cli, just confirm this with me before I implement it @benjeffery

@benjeffery
Copy link
Member

Yep!

@jeromekelleher
Copy link
Member

Agree this is the right approach

@hossam26644 hossam26644 force-pushed the print-out-split-large-numbers branch from 9b3b89c to 41b54f0 Compare May 21, 2025 09:44
@hossam26644
Copy link
Contributor Author

Is there a way to add squishing different commits to the workflow before merging rather than doing it manually? then if more changes are to be done we wont have to squish again (and I just hate to force push)?

@benjeffery
Copy link
Member

Is there a way to add squishing different commits to the workflow before merging rather than doing it manually? then if more changes are to be done we wont have to squish again (and I just hate to force push)?

There's two approaches to this, depending on context.

  1. Just push as many commits as you like to the branch of this PR, this lets people review individual commits and is useful as people can see what is changing. We then just squash and force push at the end.

  2. git commit --amend and force push to edit the last commit - useful for little changes and linting fixes that aren't useful to see separately.

You should not be scared of force pushing at all, nothing is lost as all the previous states of your branch will be in the reflog. (You can see them with git reflog).

I force push about 50% of the time, it is a normal part of the workflow.

@hossam26644
Copy link
Contributor Author

@benjeffery another review?

Copy link
Member

@benjeffery benjeffery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - a couple of nits.

@@ -12,12 +12,15 @@
associated with each individual as a numpy array.
(:user:`benjeffery`, :pr:`3153`)

- Implement thin space separation for thousands in the numbers output for html
and text.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just HTML now.

@benjeffery benjeffery enabled auto-merge May 21, 2025 13:05
@benjeffery benjeffery disabled auto-merge May 21, 2025 13:06
@benjeffery
Copy link
Member

Great, squash and we can merge.

@hossam26644 hossam26644 force-pushed the print-out-split-large-numbers branch from 4d606c5 to 90a4746 Compare May 21, 2025 13:11
@benjeffery benjeffery enabled auto-merge May 21, 2025 13:24
auto-merge was automatically disabled May 21, 2025 13:55

Head branch was pushed to by a user without write access

@hossam26644 hossam26644 force-pushed the print-out-split-large-numbers branch from 90a4746 to aceb8df Compare May 21, 2025 13:55
@benjeffery benjeffery enabled auto-merge May 21, 2025 14:00
@benjeffery benjeffery force-pushed the print-out-split-large-numbers branch from aceb8df to 9f4f8b6 Compare May 21, 2025 15:56
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 this pull request may close these issues.

Split large numbers in html/cli print out.
3 participants