-
Notifications
You must be signed in to change notification settings - Fork 58
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
Major refactoring of Diagnostics module #776
Conversation
@jdkent To avoid the breaking change, do you think it would be a good idea if instead, we return an empty contribution table when the estimator is not supported, similar to what we do when the number of clusters is 0: Lines 167 to 170 in 08cbfad
In that case, we won't need to have the extra flag return_clusters_table_only .
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #776 +/- ##
==========================================
+ Coverage 88.39% 88.46% +0.07%
==========================================
Files 40 40
Lines 4791 4761 -30
==========================================
- Hits 4235 4212 -23
+ Misses 556 549 -7
... and 2 files with indirect coverage changes 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 in Codecov by Sentry. |
yeah, returning an empty contribution table sounds okay with an explicit warning. Not introducing another argument feels good. I'm feeling between two options:
An empty clusters table makes sense because there was nothing incompatible in the code, it was just an attribute of the data, and we wouldn't want to change the output type just because nothing survived the clustering algorithm. The actual answer is that there are no clusters in the table In this current case, we know which combinations of Estimators/Diagnostics beforehand don't work, so if we return an empty table, are we saying "no studies contributed to any of the clusters"? (No we are not saying that). So I have a slight preference to return |
Thanks! |
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.
Looked it over, changes look good and you test it with one of the pairwise estimators, LGTM.
thanks! |
This is part of the refactoring issue #772, to reduce some of the duplicated code as was pointed out here #765.
Changes proposed in this pull request:
Diagnostics
, to reduce duplicated code.