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

Hypothalamus Implementation #459

Merged
merged 49 commits into from
Jun 20, 2024
Merged

Hypothalamus Implementation #459

merged 49 commits into from
Jun 20, 2024

Conversation

santiestrada32
Copy link
Contributor

New

  • Inclusion of Hypothalamus module
  • Inclusion of the hypothalamus labels into FastSurfer LUT

TODO

  • Include HypVINN in fastsurfer segmentation pipeline
  • Include T2 input. The input T2 image should be biased field corrected and co-register to the T1 (see HypVINN readme file)

@santiestrada32
Copy link
Contributor Author

I was getting an attribute error with the current import of the yacs library
Error:
AttributeError: module 'yacs' has no attribute 'config'

@dkuegler
Copy link
Member

I was getting an attribute error with the current import of the yacs library Error: AttributeError: module 'yacs' has no attribute 'config'

That looks like a typing bug.
Just put quotes around it in the typing for now

Copy link
Member

@dkuegler dkuegler left a comment

Choose a reason for hiding this comment

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

I went through the code and have some comments.

Some of the points can probably be done by someone else, but there are a couple of questions we want to fix soon, before you invest much more work. Specifically, the questions regarding bias field correction.

Docker/install_fs_pruned.sh Show resolved Hide resolved
HypVINN/config/HypVINN_axial_v1.0.0.yaml Outdated Show resolved Hide resolved
HypVINN/config/HypVINN_coronal_v1.0.0.yaml Outdated Show resolved Hide resolved
HypVINN/utils/mode_config.py Outdated Show resolved Hide resolved
HypVINN/README.md Outdated Show resolved Hide resolved
HypVINN/utils/img_processing_utils.py Outdated Show resolved Hide resolved
HypVINN/run_preproc.py Outdated Show resolved Hide resolved
HypVINN/utils/misc.py Outdated Show resolved Hide resolved
HypVINN/utils/img_processing_utils.py Outdated Show resolved Hide resolved
HypVINN/utils/img_processing_utils.py Outdated Show resolved Hide resolved
@m-reuter
Copy link
Member

please don't merge from dev, do a rebase instead. Rebase - of course - should not be done if we are officially collaborating on a branch it is checked out by other people. but if this is mainly on your fork, just rebase, which will add the dev changes before your changes and keeps the history linear.

Copy link
Member

@dkuegler dkuegler left a comment

Choose a reason for hiding this comment

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

This review was "overdue". Now it is down to me to fix run_fastsurfer.sh.

HypVINN/README.md Outdated Show resolved Hide resolved
HypVINN/README.md Outdated Show resolved Hide resolved
HypVINN/README.md Outdated Show resolved Hide resolved
run_fastsurfer.sh Outdated Show resolved Hide resolved
@dkuegler dkuegler marked this pull request as ready for review April 19, 2024 17:47
@santiestrada32
Copy link
Contributor Author

I think the branch is ready to merge. Or something else needs to be tested?

@dkuegler
Copy link
Member

I think there is one issue left that I need to resolve, which is based around conforming/pre-processing of the T2 image.

In the current code, the conversion to uchar is independent of the bias field correction. This is not necessary, the bias field script has a --uchar flag that actually incorporates this functionality exactly.

The next/other open issue is the processing if the --t2 image is given, but --no_biasfield is specified. This would mean, that the biasfield has been computed externally. That would be fine, but the conversion to uchar (and robust rescaling) is not ensured within the pipeline. This change needs a small adaptation of the conform script, I believe to also enable robust rescaling without reorientation, which is a feature that we also regularly need in other contexts.

I have a branch where both of these issues are started (actually 1 should be finished, 2 is started but not tested yet).

Finally, there used to be a sphinx documentation problem, that I was supposed to fix, but did not get around to yet. So both are not really critical issues, but more "side-issues".

@santiestrada32
Copy link
Contributor Author

I think it would be nice to merge my Branch to dev before OHBM ( Monday, June 24), as I will be presenting HypVINN.

In the preproc.py, before I registered T1 and T2, I ensured that the T2 image was converted to uchar and that robust rescaling was applied. I don't do this if they put none as a registration step, so I agree that improving the T2 conform will be nice.

Let me if I can help with something

@m-reuter
Copy link
Member

We can also merge now and do the rest as separate PRs?

santiestrada32 and others added 6 commits June 20, 2024 14:36
without __pycache__ files
include HypVINN labels
Create Main function for N4 bias correct
include fs bin for registration
Include copyright statements in Hypvinn files
update run pipeline
test fs
santiestrada32 and others added 23 commits June 20, 2024 14:36
Add hypvinn url file
- rename mode "multi" to "t1t2"
- reformatting for line length
- replace single quotes to double quotes
- add typing information
- clean up docstrings
- replace os.path with pathlib.Path
Fix typo in N4_bias_correct.py
- remove/rename changed filenames
- format table

HypVINN/config/checkpoint_paths.yaml
- add config

HypVINN/data_loader/data_utils.py
- fix typing and formatting

HypVINN/utils/checkpoint.py
- fix YAML_DEFAULT

HypVINN/utils/mode_config.py
- set default values for get_hypvinn_mode

HypVINN/inference.py
- fix inclusion of ModalityMode

HypVINN/run_prediction.py
- move HypVINN/run_pipeline.py into run_prediction.py
- fix typing, e.g. FileBasedHeader
- fix function parameters
- add help text to hypo_segfile argument
- fix passing of t1_path and t2_path
- various other changes
- Fix errors in t1t2 mode
- Fix qc_snaps argument in run_fastsurfer.sh
- Fix reg_mode test in run_fastsurfer.sh
update model weight (version 1.1) for hypvinn
update download script
Convert the T2 image to unit8 to save hard disc space for registration
- set --uchar for N4_bias_correct.py
- `conform.py --no_force_(img_size|vox_size|lia) --t2 <arg>` if --no_biasfield
Copy the T2 file to $SUBJECTS_DIR/$SID/mri/orig/T2.001.mgz
- Added docstring for functions in HypVINN
- adding space before colon separating parameter name and type
…of a constant

Make saving a main thread function instead of a future/task
- Fix for bash-3.2 (|& => 2&>1 |)
- Add warning if T2 is passed, but biasfield is skipped --no_biasfield
- add comment paper vs. retrained weights.
- add hypothal module in main README
- fix sphinx errors
- add an additional message in fix_links to debug errors
- add the hypvinn script documentation
@dkuegler
Copy link
Member

I am going to merge this now. This should be ready for the base use case, but I want to fix the robust scaling of the T2 image. This can be a different PR though.

@dkuegler dkuegler merged commit a4092db into Deep-MI:dev Jun 20, 2024
2 checks passed
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.

5 participants