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.

Sign up for GitHub

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

Jump to bottom

Dealing with derived volumes in harmonization #189

Merged
merged 1 commit into from
Mar 15, 2022
Merged

Dealing with derived volumes in harmonization #189

merged 1 commit into from
Mar 15, 2022

Conversation

melhemr
Copy link
Contributor

@melhemr melhemr commented Mar 15, 2022 •
edited
Loading

This PR handles cases in which DERIVED ROI volumes are included in the harmonization model. It also fixes a bug that has to do with age limits in harmonization

@melhemr
Copy link
Contributor Author

melhemr commented Mar 15, 2022

@AbdulkadirA I included the dataset/model used to make checks on this PR. The model, in particular, is useful because it includes derived harmonized volumes in the key model['ROIs']. I have saved them to my comp_space found here:

/cbica/home/melhemr/comp_space/Mathilde/

data_for_harmonization1994_good.pkl is the data
phenom_hc_harmonize_iStaging_modelTEST.pkl is the model

@melhemr
Copy link
Contributor Author

melhemr commented Mar 15, 2022

@AbdulkadirA also, make sure any istaging dataset you match with phenom_hc_harmonize_iStaging_modelTEST.pkl includes a 'UseForComBatGAMHarmonization' column so that harmonized volumes will be calculated out-of-model

AbdulkadirA
AbdulkadirA suggested changes Mar 15, 2022
View reviewed changes
Copy link
Contributor

@AbdulkadirA AbdulkadirA left a comment

Choose a reason for hiding this comment

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

@melhemr Thanks. Please use logger.info() instead of print() for better control.

NiBAx/plugins/harmonization/harmonization.py Outdated Show resolved Hide resolved
NiBAx/plugins/harmonization/harmonization.py Outdated Show resolved Hide resolved
NiBAx/plugins/harmonization/harmonization.py Outdated Show resolved Hide resolved
NiBAx/plugins/harmonization/harmonization.py Outdated Show resolved Hide resolved
NiBAx/plugins/harmonization/harmonization.py Outdated Show resolved Hide resolved
@AbdulkadirA
Copy link
Contributor

The issue with overstepping bounds is resolved.

AbdulkadirA
AbdulkadirA suggested changes Mar 15, 2022
View reviewed changes
Copy link
Contributor

@AbdulkadirA AbdulkadirA left a comment

Choose a reason for hiding this comment

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

@melhemr Thanks. Just a small detail. There should be no line break in the log message. If you want display multiple lines, please use logger.info() repeatedly.

NiBAx/plugins/harmonization/harmonization.py Outdated Show resolved Hide resolved
AbdulkadirA
AbdulkadirA approved these changes Mar 15, 2022
View reviewed changes
Copy link
Contributor

@AbdulkadirA AbdulkadirA left a comment

Choose a reason for hiding this comment

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

@melhemr Thanks.

@AbdulkadirA AbdulkadirA merged commit 3bdaf6b into CBICA:main Mar 15, 2022
@melhemr melhemr deleted the mathilde_issues branch May 31, 2022 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AbdulkadirA AbdulkadirA AbdulkadirA approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@melhemr @AbdulkadirA