Fix bug in spherically project wrapper#788
Merged
m-reuter merged 5 commits intoDeep-MI:devfrom Feb 17, 2026
Merged
Conversation
This Commit adds the missing forward_output member function of MessageBuffer, which is used in the fallback case of spherically project and run_tools.Popen.forward_output. Harmonize Popen.__init__ with subprocess.__init__
m-reuter
reviewed
Feb 13, 2026
- the FastSurfer spherically_project.py uses full file names - the FreeSurfer fallback binary uses the SUBJECTS_DIR environment variable, --sid and internally hard coded file names.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes fallback-path failures when spherically_project_wrapper.py is invoked from recon-surf.sh, and aligns FastSurferCNN.utils.run_tools.Popen’s constructor with subprocess.Popen.
Changes:
- Replace
--sdirwith--sdand correctly construct surface paths from$SUBJECTS_DIR/<subject>/surf. - Add
MessageBuffer.forward_output()to supportPopen.forward_output()without raising an attribute error. - Adjust
run_tools.Popen.__init__to accept only the single positionalargsparameter (likesubprocess.Popen).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| recon_surf/spherically_project_wrapper.py | Switch wrapper CLI to --sd and fix path/env usage for the fallback execution path. |
| recon_surf/recon-surf.sh | Update wrapper invocation to pass --sd "$SUBJECTS_DIR" instead of --sdir "$sdir". |
| FastSurferCNN/utils/run_tools.py | Add output-forwarding on MessageBuffer and tighten Popen.__init__ signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fix default values and required settings of spherically_project_wrapper.py to ensure the parameters are always validated.
dkuegler
force-pushed
the
fix/forward-output
branch
from
548d5f9 to
e2f3288
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes two bugs in the use of spherically_project_wrapper.py. Both of them only appear if the baseline python implementation fails.
spherically_project_wrapper.pyuses a--sdargument, but the value was previously passed as empty (the used variable did not exist).MessageBuffer:forward_output, which forwards stdout and stderr content through the wrapper to the python stdout. This function missing raised an error.Additionally, the PR fixes a near-irrelevant signature-inconsistency between the
run_toolsextension ofPopen.__init__and the underlyingsubprocess.Popen.__init__, wheresubprocess.Popenonly accepts one positional parameter, butrun_tools.Popenaccepted multiple ... only to then pass that tosubprocess.Popen, where it would be invalid.Todo:
test in bigger dataset to cover both spherically project implementations