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 dataset data_normalize_metric_pam50
#4045
Conversation
Will it be downloaded with the SCT installation?
Yes it is fine! |
Not by default. Should it be? If so, we just need to add it to Lines 684 to 689 in 158abd1
and to spinalcordtoolbox/install_sct.bat Lines 97 to 100 in 158abd1
|
@@ -159,7 +159,13 @@ | |||
"https://github.com/ivadomed/multimodal-registration/releases/download/r20220512/models.zip" | |||
], | |||
"default_location": os.path.join(__sct_dir__, "data", "deepreg_models"), | |||
} | |||
}, | |||
"data_normalize_metric_pam50": { |
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.
Question for the reviewer(s): is
data_normalize_metric_pam50
the name we want to use for this dataset?
Some nitpicks:
- Elsewhere in
sct_download_data
, we usePAM50
(capitalized), so it might be good to unify that. - Part of me wants to use a name that represents a noun, similar to the other dataset names? For example, rather than "data normalize metric pam50", we would say "PAM50-normalized metrics", which could then become the dataset name
PAM50_normalized_metrics
.
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 am fine with renaming the repo and folder!
"https://github.com/spinalcordtoolbox/data_normalize_metric_pam50/archive/refs/tags/r20230221.zip" | ||
], | ||
"default_location": os.path.join(__sct_dir__, "data", "data_normalize_metric_pam50"), | ||
}, | ||
} | ||
|
||
|
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.
Side-question: Would it be beneficial to combine this PR with #4003?
The reason I suggest this is because of the following scenario:
- We merge this PR to master.
- #4003 is updated with master to include the new dataset link.
- We start playing around with #4003 to compute compression metrics from the CSV dataset.
- During testing, we identify some flaw or concern with the dataset (perhaps some issue with how the metrics were generated via #3977).
- We need to regenerate the dataset and create a new release link, and thus a new change to
master
.
By comparison, if this dataset link were to be included as part of #4003, then it could potentially save a bit of clutter on master, and keep related development grouped in the same location, since any changes to the dataset would be part of #4003's development.
(But, maybe the scenario I describe is unlikely, or easy enough to handle if something goes wrong?)
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.
yes, we can include this in #4003, I agree!
OK great! We discussed this in the sct_dev meeting and with the project team for the compression metrics, we want it within the sct installation! |
Fixes #4044.
For testing, I successfully downloaded the data with:
Question for the reviewer(s): is
data_normalize_metric_pam50
the name we want to use for this dataset?