Skip to content
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

Merged
merged 12 commits into from Mar 17, 2023

Conversation

JulioAPeraza
Copy link
Collaborator

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:

  • Create a new base class Diagnostics, to reduce duplicated code.
  • In addition, we plan to change the AttributeError to a Warning when using pairwise Estimators and return only the clusters table as suggested here Add CBMA workflow #761 by @jdkent.

@JulioAPeraza JulioAPeraza changed the title Major Refactoring of Diagnostics module Major refactoring of Diagnostics module Mar 15, 2023
@JulioAPeraza JulioAPeraza added diagnostics Issues/PRs related to the diagnostics module. enhancement New feature or request refactoring Requesting changes to the code which do not impact behavior and removed enhancement New feature or request labels Mar 15, 2023
@JulioAPeraza
Copy link
Collaborator Author

@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:

if clusters_table.shape[0] == 0:
LGR.warning("No clusters found")
contribution_table = pd.DataFrame(columns=["id"], data=rows)
return contribution_table, clusters_table, label_maps

In that case, we won't need to have the extra flag return_clusters_table_only.

@codecov
Copy link

codecov bot commented Mar 15, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.07 🎉

Comparison is base (08cbfad) 88.39% compared to head (758b08f) 88.46%.

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     
Impacted Files Coverage Δ
nimare/diagnostics.py 99.15% <100.00%> (+5.23%) ⬆️

... 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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jdkent
Copy link
Member

jdkent commented Mar 15, 2023

yeah, returning an empty contribution table sounds okay with an explicit warning. Not introducing another argument feels good.

I'm feeling between two options:

  • return empty contribution table
  • return None

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 None for the contribution table for these predetermined combinations because None signals to me that "we could not make a reasonable output for some reason, but someday in the future a reasonable output could exist here".

@JulioAPeraza
Copy link
Collaborator Author

Thanks!
I've added the suggestions and documented the new changes. I also added a new test to improve coverage.

@JulioAPeraza JulioAPeraza mentioned this pull request Mar 17, 2023
Copy link
Member

@jdkent jdkent left a 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.

@jdkent jdkent merged commit ad09e4f into neurostuff:main Mar 17, 2023
@JulioAPeraza
Copy link
Collaborator Author

thanks!

@JulioAPeraza JulioAPeraza deleted the ref-diagnostics branch March 23, 2023 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
diagnostics Issues/PRs related to the diagnostics module. refactoring Requesting changes to the code which do not impact behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants