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
Add default synthetic sample size to DiagnosticReport #248
Conversation
Codecov ReportBase: 74.35% // Head: 74.42% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #248 +/- ##
==========================================
+ Coverage 74.35% 74.42% +0.06%
==========================================
Files 74 74
Lines 2924 2932 +8
==========================================
+ Hits 2174 2182 +8
Misses 750 750
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@@ -33,6 +33,10 @@ class DiagnosticReport(): | |||
'Boundaries': [BoundaryAdherence], | |||
} | |||
|
|||
_METRIC_ARGS = { |
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 to make this a class attribute instead of an instance attribute? I just wonder because if you change the values in one then I think the next DiagnosticReport
created will also have the changed value
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.
Ah I see, I can move it to an instance attribute, good call
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.
LGTM!
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.
I left a suggestion on renaming the args
to metric_args
other than that it looks good to merge.
|
||
for metric in tqdm.tqdm(metrics, desc='Creating report'): | ||
metric_name = metric.__name__ | ||
try: | ||
args = self._metric_args.get(metric_name, {}) |
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 parameter args
confuses me since it's a special key or in some cases it can be reserved for the method definition eg: def func(*args, **kwargs)
.I would suggest to change args
to something like metric_args
or similar just to avoid this type of confusion.
No description provided.