-
Notifications
You must be signed in to change notification settings - Fork 42
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
Include subsegment information when aggregating split statistics #433
Include subsegment information when aggregating split statistics #433
Conversation
if self.is_name_default(): | ||
stats_type = "unk" |
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.
The old code was checking for bin
types too.
I'm not sure if we should keep that check tho, naming the section itself could be considered as it being known, idk.
What do you think @ethteck ?
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.
yeah I think considering it known if it's named is totally fine, personally
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 this! Just one q
@@ -1,4 +1,4 @@ | |||
from ...segtypes.segment import Segment | |||
from ...segtypes.segment import Segment, SegmentType |
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.
Is there a reason for this change?
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.
Just a type alias to make the types for the code I introduced more self-documenting — certainly not necessary
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.
ahh I hadn't understood the full context of it. thank you!
if self.is_name_default(): | ||
stats_type = "unk" |
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.
yeah I think considering it known if it's named is totally fine, personally
@forestbelton before I merge, could you add an entry to the changelog for this? Under 0.32.1 |
Fixes #432
Description
Extend
util.Statistics
so that it breaks out statistics for segments which include subsegments.I extended the base
Segment
type to include a propertystatistics
for querying subsegment statistics, defaulting to the current segment information only.CommonSegGroup
overrides this to provide aggregated statistics based on all of its subsegments.Caveat
This change has the effect that a segment inheriting from
CommonSegGroup
will not be itself included in the statistics output. An example is given in the test output below.I'm not sure if this is desired behavior, but I couldn't figure out a way to incorporate both subsegment information and retain the segment name without changing the output into some kind of tree format.
Test output
Using the following segment configuration:
Before:
After: