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

Support for edits #591

Merged
merged 9 commits into from
Dec 19, 2024
Merged

Support for edits #591

merged 9 commits into from
Dec 19, 2024

Conversation

dkuegler
Copy link
Member

This PR adds support for edits in FastSurfer.

Edits are enabled by the --edits flag for run_fastsurfer.sh.

Edits supported:
<asegdkt_segfile> via <asegdkt_segfile-w/o-ext>.manedit.
white matter segmentation (wm.mgz) via wm.manedit.mgz
white matter control points (via the standard FreeSurfer control points file)
talairach registration via mri/mri/transforms/talairach.xfm (overwriting talairach registration via mri/mri/transforms/talairach.xfm)
brain.finalsurfs.mgz via brain.finalsurfs.manedit.mgz

This PR also move the allow_root check into functions.sh and introduces run_it and run_it_cmdf function variants that are more clear wrt. argument splitting (see SC2206, SC2207 and similar).

@m-reuter
Copy link
Member

Hi, thanks for the pr. It would be great to fix the spelling errors and also add a readme for edits, which can be pretty short in the first iteration, with a 1-2 sentence description of each possible edit.

@dkuegler
Copy link
Member Author

Spelling should be fixed, documentation will need to be a future thing for now.

recon_surf/recon-surf.sh Outdated Show resolved Hide resolved
run_fastsurfer.sh Outdated Show resolved Hide resolved
@dkuegler dkuegler marked this pull request as draft October 10, 2024 16:38
@dkuegler
Copy link
Member Author

This branch is currently a mess of fixes and features for 6 different things. Those need to be separated into individual branches and PRs.

At a minimum, this version seems to work for longitudinal processing (still unclear for edits).

@dkuegler dkuegler force-pushed the feature/edits branch 2 times, most recently from 21f9e19 to 82cd0aa Compare October 11, 2024 14:10
@m-reuter
Copy link
Member

I rebased this to current dev which disentangles the edits from the other changes. Please check :

  • that everything is as wanted and
  • no purposeful changes from the earlier commits are undone

Also update your local with a hard reset.

@m-reuter m-reuter mentioned this pull request Nov 2, 2024
@dkuegler dkuegler force-pushed the feature/edits branch 3 times, most recently from 69efedc to 0000b25 Compare November 14, 2024 10:23
@dkuegler dkuegler marked this pull request as ready for review November 19, 2024 17:35
@dkuegler dkuegler requested a review from m-reuter November 19, 2024 17:35
@dkuegler dkuegler force-pushed the feature/edits branch 7 times, most recently from 0716df6 to 9aace28 Compare November 20, 2024 17:40
@dkuegler
Copy link
Member Author

Maybe there should be a check if the fastsurfer versions are the same between the original run and the rerun? @m-reuter

If this is just a warning or a message this will likely get ignored.
If this is a failure, it might get more annoying than not.
Solution might be to write a message to the log and let the user confirm it (enter).
But if nothing is attached to stdin, the process proceeds no matter what so only interactive sessions stop (this is how install scripts sometimes solve this)...

@m-reuter
Copy link
Member

Maybe there should be a check if the fastsurfer versions are the same between the original run and the rerun?

I think warning message in the logs is fine. It should only stop if versions are not compatible any longer (and it would fail somewhere during processing). Otherwise, if someone edits things, that is changing more than most of our versions.

doc/overview/EDITING.md Outdated Show resolved Hide resolved
doc/overview/EDITING.md Outdated Show resolved Hide resolved
doc/overview/EDITING.md Outdated Show resolved Hide resolved
3. Brain extraction by editing `<subject_dir>/mri/brain.mgz` (delta to `brain.auto.mgz`) -> [asegdkt_segfile](#asegdkt_segfile)
4. Subcortical segmentation: `<subject_dir>/mri/aseg.presurf.mgz` (delta to `aseg.presurf.auto.mgz`) -> [asegdkt_segfile](#asegdkt_segfile)
5. CRS seeds: PONS, CC, LH, RH, watershed => seed files (`<subject_dir>/scripts/seed-(pons|cc|lh|rh|ws).crs.man.dat`) -> [asegdkt_segfile](#asegdkt_segfile)
6. WM control points: `<subject_dir>/tmp/control.dat` -> [asegdkt_segfile](#asegdkt_segfile)
Copy link
Member

Choose a reason for hiding this comment

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

hm, we could allow them to create control points which should be used by recon-surf when crating wm.mgz?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we could, but there are some comments somewhere why we would not want to do that. I think I did not really understand those comment but just assumed you had decided against this on principle.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, I am not really sure how these labels would propagate back into aseg.mgz and aparc.DKTatlas+aseg.mgz

Copy link
Member

Choose a reason for hiding this comment

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

They would not, they are only used to increase intensity in thin WM regions to avoid loosing a gyrus or parts of a lobe (usually temporal lobe) due to low grey white contrast there. Let's discuss.

dkuegler and others added 8 commits December 19, 2024 17:25
- Adds the --edits flag
- Add additional log messages for edits
- Support for manedit files for asegdkt_segfile.
- Also propagate asegdkt_segfile into aseg_segfile and mask_name

recon_surf/recon-surf.sh
- Adds the --edits flag
- Add additional log messages for edits
- Change check for existing run into a message instead a fail criteria in edits mode
- Do not delete logfile in edits mode
- Make mask and aseg_noCC required for edits mode (otherwise it might get unclear what should or was already done between run_fastsurfer and recon-surf)
- Add manedit "search" for mask and asegdkt_segfile.

recon_surf/talairach-reg.sh
- Add an extra argument (new argument #3 to indicate whether talairach.reg should support edits)
- Support for edits: some messages and if in edit mode, only run talairach registration if mri/transforms/talairach.xfm and mri/transforms/talairach.auto.xfm are the same (so there are no edits) -- thus edits in talairach.xfm superceed automatic results
- talairch-reg.sh will now fail if mri/transforms/talairach.xfm exists, but the script is not running in edits mode (independent if talairach.auto.xfm is the same or not).

recon_surf/functions.sh
- add the add_file_suffix function, which adds a file suffix (for example manedits) to a file name
Co-authored-by: David Kügler <[email protected]>
recon_surf/recon-surf.sh Outdated Show resolved Hide resolved
@m-reuter m-reuter merged commit faca489 into Deep-MI:dev Dec 19, 2024
2 checks passed
@dkuegler dkuegler deleted the feature/edits branch December 20, 2024 16:19
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.

3 participants