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

rl/revise1 height weight analysis #275

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

renelabounek
Copy link
Contributor

@renelabounek renelabounek commented Dec 12, 2023

Fixes #271 and #272
Extends results as required by co-authors in the manuscript

May partly fix #273 and #274

Still work in progress.

@renelabounek
Copy link
Contributor Author

renelabounek commented Dec 13, 2023

Commit 9cd6af1 may fix looking for manual segmentation and labeling in T1w, T2w and T2star and potentially fix #273 and partly #274. If T1w or T2w scans are not in the same space (after RPI_r operations) as the seg-manual.nii.gz file, then this fix may not be optimal.

Missing:
There is 267 manually labeled discs (see #274). However, process_data.sh can not use such file in manual labeling file. Analysis is running. Now I see that labeling failed in 4 scans so far. Utilizing labelend discs may fix issue with failing labeling.

Labeling error messages:
1/ Automatic C2-C3 detection failed. Please provide manual label with sct_label_utils

2/ Labels {5} were lost during straightening transform. This can be caused by the labels being outside the ROI of the spinal cord segmentation. Please make sure all labels are within the ROI of the spinal cord segmentation.

3/ During disc labeling, center of mass calculation failed due to discontinuities in segmented spinal cord; please check the quality of your segmentation. Using interpolated centerline as a fallback. Vertebral detection failed: Center of mass can't be calculated on empty arrays.

@renelabounek
Copy link
Contributor Author

@valosekj and @jcohenadad, can you please merge the code into spine-generic master? I think we are in the stage when we can make a release for the manuscript. But there are some conflicts in process_data.sh which I am not allowed to fix.

@valosekj
Copy link
Contributor

valosekj commented Mar 26, 2024

But there are some conflicts in process_data.sh which I am not allowed to fix.

The conflicts for the process_data.sh script are mainly caused by the update of the naming convention done in #276. We will discuss the best strategy how to deal with this during our next meeting (April 3) and get back to you.

@renelabounek
Copy link
Contributor Author

The conflicts for the process_data.sh script are mainly caused by the update of the naming convention done in #276.

That does not seem like hard issue to fix, but it is true that it would be good if the proces_data.sh script would work with the database version used in the current manuscript.

Regarding comments from co-authors. I need to add into hte code data normality testing; and Spearman correlation coefficients may also be added.

@valosekj
Copy link
Contributor

valosekj commented Apr 3, 2024

Okay, we discussed how to publish a release of your code during the 2024-04-03 SCT dev meeting. We think a good way to go is to publish a release directly from your branch. In other words, we will not merge your branch to master (due to conflicts).
So let me know when the branch is ready, and I'll publish the release.

@renelabounek
Copy link
Contributor Author

Okay, we discussed how to publish a release of your code during the 2024-04-03 SCT dev meeting. We think a good way to go is to publish a release directly from your branch. In other words, we will not merge your branch to master (due to conflicts).
So let me know when the branch is ready, and I'll publish the release.

I do not follow what that practicaly mean, but I thnink my branch:
https://github.com/renelabounek/spine-generic/tree/rl/revise1-height-weight-analysis
contains final code which may be used for the release.

Is it in a way you need it? I would rather not to merge it into my master branch which I try to keep same as the spine-generic:master version is.

@valosekj
Copy link
Contributor

I do not follow what that practicaly mean, but I thnink my branch: https://github.com/renelabounek/spine-generic/tree/rl/revise1-height-weight-analysis contains final code which may be used for the release.

Thanks for letting me know! I tried to draft a new release, but the "drop-down release menu" only shows branches from this repo, not from your fork. @mguaypaq, any ideas on how to create a release from a fork repo? In the worst case, we can create a release within your fork.

@renelabounek
Copy link
Contributor Author

BTW are there so huge conflicts in the process_data.sh? As I remember it should be just several rows with the file naming to derivatives file. Right? What about putting several if else conditions which would make the code work for both data-multisubject database releases? It is not just this code which would stop to work if the same datbase relase is utilized for any analysis. Right?

@mguaypaq
Copy link
Member

@valosekj There's now a copy of the branch rl/revise1-height-weight-analysis in the spine-generic/spine-generic repository, so you should be able to create the release. (I used the command line to manually pull it from the renelabounek fork and push it upstream.)

Also, if more commits get added to the renelabounek fork in the future, you should be able to open a pull request to update the rl/revise1-height-weight-analysis branch in spine-generic (now that the branch exists in both copies of the repository).

@renelabounek Putting a bunch of if/else conditions in the code would mean that we are committing to supporting the old file naming scheme even as the dataset repository keeps evolving, which is a cost that increases over time. And it wouldn't gain much, because any sort of reproducible analysis should give specific versions for both the code and the data. By making a special release of the code from your branch, we ensure that the exact code and data used in your published analysis will always be available.

@renelabounek
Copy link
Contributor Author

From my point of view, you can make the release. Thanks :-)

@valosekj
Copy link
Contributor

valosekj commented Apr 26, 2024

There's now a copy of the branch rl/revise1-height-weight-analysis in the spine-generic/spine-generic repository, so you should be able to create the release.

Great, thank you @mguaypaq!

I drafted a new release: spine-generic height-weight-analysis:

image

To make explicit that this release is related to the height and weight analysis, I used height-weight-analysis for the release tag and release title.
Please let me know if everything is okay. If so, I will publish the release!

Update: I just realized that it would probably be a good idea to cross-link the appropriate release of the spine-generic multi-subject dataset. I'll include the link in the release description.

@renelabounek
Copy link
Contributor Author

renelabounek commented Apr 26, 2024

I drafted a new release: spine-generic height-weight-analysis.

I am not allowed to see it. Or wrong link as I get 404 error. If the last commit in the relase is 51c3145. Then, I believe everything is fine and you have green to go from me. ;-)

@renelabounek
Copy link
Contributor Author

Is the release online? I still see only Error 404 Page not found.

@valosekj
Copy link
Contributor

Is the release online? I still see only Error 404 Page not found.

Yes! https://github.com/spine-generic/spine-generic/releases/tag/height-weight-analysis

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GM under-segmented on Phillips data in data-multi-subject process_data.sh - dorsal columns labels
3 participants