Conversation
Closes #726. `cortex.freesurfer.import_subj` now finishes by cutting and flattening the subject's surfaces with autoflatten (https://gallantlab.org/autoflatten) and importing the resulting flatmaps, so that a freshly imported subject is immediately usable for flatmaps without a manual cutting session. The step is controlled by two new keyword arguments: - `autoflatten=True` runs it (pass False to skip) - `autoflatten_args=None` passes extra arguments through to `autoflatten run`, e.g. `["--backend", "freesurfer"]` Because it takes 15-30 minutes for both hemispheres, a warning says so before starting, and it is skipped silently when both flat patches already exist (autoflatten does not recompute them). It runs last, after everything else has been imported successfully, so a failure there does not leave a half-imported subject. autoflatten is an optional dependency, added as the `autoflatten` extra. When it is not installed, `import_subj` warns and skips the step rather than failing, so existing workflows keep working after an upgrade. That check happens up front, before any conversion work, rather than after several minutes of importing. The implementation lives in the private `cortex/_autoflatten.py`, which shells out to `python -m autoflatten.cli run <subject dir>` using the interpreter running pycortex, so it does not depend on the CLI being on $PATH. It imports `cortex.freesurfer` lazily inside its functions, since `freesurfer` imports it at module level. `autoflatten_subject` is re-exported as `cortex.freesurfer.autoflatten_subject`, which is the public name for running the step on its own later. Tests cover the command construction and argument pass-through, the `import_flat` wiring, the missing-package and missing-output failure paths, the warning behaviour, and the plumbing of both keyword arguments through `import_subj`. A `slow` end-to-end test really runs autoflatten against a freesurfer subject named by $PYCORTEX_TEST_FS_SUBJECT (the bundled S1 is a pycortex filestore entry, not a freesurfer subject directory, so it cannot be flattened); it is deselected by default and self-skips when freesurfer or autoflatten is unavailable. Also drops three unused imports and a stray semicolon in cortex/freesurfer.py, flagged by ruff on the touched file, and adds the `import sys` that its `__main__` block was already using. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mvdoc
marked this pull request as draft
September 1, 2026 23:12
Two fixes from a review of the autoflatten import step: - `autoflatten run` resolves the subject and the fsaverage template it maps the cuts from by name through `$SUBJECTS_DIR`, but the subprocess inherited whatever `$SUBJECTS_DIR` happened to be set to (or nothing at all), while flattening read `?h.fiducial` from `freesurfer_subject_dir`. Pass the resolved directory through `env=`, matching what `import_subj` already does for its other subprocesses. - The expected patch files and the `import_flat` call both assumed two hemispheres, so `autoflatten_args=["--hemispheres", "lh"]` ended a successful run with `IOError: autoflatten did not produce the expected flat patch file(s)`. `_get_hemispheres` now parses that option (in both its `--hemispheres lh` and `--hemispheres=lh` spellings) and drives both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Ba4KQLQmYhb2ypXAFT1k8
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Forced overwrite runs can silently begin a 15–30 minute operation, and the runtime warning recommends an unsupported parameter for direct API calls.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Integrates automatic surface flattening into FreeSurfer subject imports.
Changes:
- Adds optional autoflatten dependency and integration.
- Documents the new workflow and API.
- Adds unit and opt-in end-to-end tests.
| File | Description |
|---|---|
pyproject.toml |
Adds autoflatten extra. |
docs/database.rst |
Documents automatic flattening. |
docs/api_reference_flat.rst |
Exposes the new API. |
cortex/freesurfer.py |
Integrates flattening into imports. |
cortex/_autoflatten.py |
Implements autoflatten execution. |
cortex/tests/test_freesurfer.py |
Tests import integration. |
cortex/tests/test_autoflatten.py |
Tests autoflatten behavior. |
AGENTS.md |
Documents architecture and slow tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
autoflatten skips patches that are already on disk, so the 15-30 minute runtime warning was suppressed whenever both flat patches existed. But `autoflatten_args=["--overwrite"]` makes autoflatten redo them anyway, which started a long run silently. Add `_forces_overwrite` and include it in the warning condition. Also reword RUNTIME_WARNING: it is issued by `autoflatten_subject`, which has no `autoflatten` argument, so pointing at a bare `autoflatten=False` led users to pass an unsupported keyword. Attribute it to `import_subj`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.


Closes #726.
What
cortex.freesurfer.import_subjnow finishes by cutting and flattening the subject's surfaces with autoflatten and importing the resulting flatmaps, so a freshly imported subject is immediately usable for flatmaps without a manual cutting session.Two new keyword arguments control it:
The step is also available on its own, for subjects imported earlier or with
autoflatten=False:Why these choices
It warns before starting. A full run takes 15-30 minutes for both hemispheres, so a
UserWarningsays so up front. It is suppressed when both flat patches already exist, since autoflatten skips patches it has already produced — matching the issue's "if it's not already present". Passautoflatten_args=["--overwrite"]to force a redo.It runs last. After the database re-init, once everything else has been imported successfully, so a flattening failure does not leave a half-imported subject. It also has to run after
make_fiducial, since?h.fiducialis the base surface autoflatten picks up.A missing autoflatten warns rather than raises. autoflatten is an optional dependency (added as the
autoflattenextra, sopip install "pycortex[autoflatten]"). Raising would turn a previously-workingimport_subjcall into a hard failure on upgrade, so instead it warns and skips. That check happens before any conversion work, not after several minutes of importing, so you find out immediately. Callingautoflatten_subjectdirectly does raiseImportError.It shells out rather than importing. autoflatten exposes a CLI, not a Python API, so this runs
python -m autoflatten.cli run <subject dir>viasys.executable— the interpreter running pycortex, so it does not depend on the CLI being on$PATH. Output is streamed straight to stdout/stderr, since you will want to watch a 30-minute job.Layout
The implementation lives in the private
cortex/_autoflatten.py. It importscortex.freesurferlazily inside its functions, sincefreesurferimports it at module level.autoflatten_subjectis re-exported ascortex.freesurfer.autoflatten_subject, which is the public name and where the docs point.Tests
cortex/tests/test_autoflatten.pycovers the command construction and argument pass-through, theimport_flatwiring, the missing-package and missing-output failure paths, and the warning behaviour;test_freesurfer.pycovers the plumbing of both keyword arguments throughimport_subj. These run in about a second and need neither FreeSurfer nor autoflatten, so they work in CI.There is also a
slowend-to-end test that really runs the pipeline:It is deselected by default (
pytest.inialready sets-m "not slow") and self-skips unless FreeSurfer is sourced withfsaveragein$SUBJECTS_DIR, autoflatten is installed, and$PYCORTEX_TEST_FS_SUBJECTnames a recon-all'd subject. It cannot use the bundled S1: that is a pycortex filestore entry (gifti surfaces), not a FreeSurfer subject directory, and autoflatten needs?h.sphere.regplusmri_label2labelto map the fsaverage template cuts onto the subject. The test mirrors the named subject into a throwaway$SUBJECTS_DIRwith symlinks (withsurf/a real directory of symlinked files, so the patches land in the copy) and passesimport_flatmaps=False, so it touches neither the real FreeSurfer subject nor your filestore.