Skip to content

Small stats fixes - #94

Open
alexhroom wants to merge 4 commits into
mainfrom
93-bad-counting
Open

alexhroom wants to merge 4 commits into
mainfrom
93-bad-counting

Conversation

@alexhroom

@alexhroom alexhroom commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Fixes #93. There was an erroneous extra count per frame added in #65. Not sure where that came from, but this PR removes it and adds a check to the histogram tests that n is correct.

Also fixes #98 by changing the histograms to use nanoseconds for time ranges internally.

@alexhroom alexhroom changed the title Fix count and add count checking to tests Small stats fixes Aug 27, 2026
Comment thread src/stats.rs
let repr = hist.__repr__();

assert!(repr.contains("time range 0.5, 2.5"));
assert!(repr.contains("time range 0.5μs - 2.5μs"));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

the units used in this label don't match those above (nano), I think having it in microseconds is more intuitive, @wilkojohnny what do you think?

@alexhroom alexhroom Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

the display string is in microseconds: see Histogram.__repr__

    pub fn __repr__(&self) -> String {
        let shape = self.hist.shape();
        let mut string = format!(
            "Histogram with:\n  time range {}μs - {}μs",
            (self.min_time.to_micros()),
            (self.max_time.to_micros())

I kept all user interface stuff in microseconds, it just uses nanoseconds internally to avoid unit conversion in the histogram calculation.

@github-actions github-actions Bot added the Has Conflicts This PR has a merge conflict. label Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

👋 Hi, @alexhroom,

Conflicts have been detected against the base branch. Please rebase your branch against the base branch.


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

@github-actions github-actions Bot removed the Has Conflicts This PR has a merge conflict. label Sep 7, 2026
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.

Remove unnecessary time conversion in histograms n counted wrong

2 participants