Skip to content

docs: correct the sulcus install instructions, which destroyed existing sulci#657

Open
jackgallant wants to merge 1 commit into
mainfrom
claude/fix-sulcus-merge-docs
Open

docs: correct the sulcus install instructions, which destroyed existing sulci#657
jackgallant wants to merge 1 commit into
mainfrom
claude/fix-sulcus-merge-docs

Conversation

@jackgallant

Copy link
Copy Markdown
Contributor

Follow-up to #656. Docs-only, no code touched.

The bug

The roidraw page tells users to "Merge the fragment into the subject's overlays.svg". Taken
literally — which is how it reads — that means appending the exported <g id="sulci"> layer to the
subject's overlay file. But SVGOverlay.__init__ keys its layers by inkscape:label:

for layer in self.svg.getroot().findall("{%s}g"%svgns):
    layer = Overlay(self, layer)
    self.layers[layer.name] = layer      # <- a second "sulci" layer replaces the first

So the appended layer silently replaces the subject's own, and every sulcus already in that file
becomes invisible to db.get_overlay(), quickflat, and the WebGL viewer. Nothing errors; the
sulci just stop existing.

The correct instruction is to copy the individual <g inkscape:label="…"> shape groups into the
subject's existing #sulci_shapes group. This PR says so, in a .. warning::, with a worked
example showing which element to copy.

Two more corrections, both from pycortex-roidraw v0.4.1

The page was written against v0.4.0, whose exporter has since been fixed. It now describes output
that no longer exists:

  • The export is no longer a bare fragment. It is a standalone SVG document declaring the svg
    and inkscape namespaces. The old fragment used the inkscape: prefix without declaring it, so
    ET.parse failed with unbound prefix and no XML parser — Python, lxml, a browser, Inkscape —
    could open the file at all.
  • The exported sulci_labels layer is empty, and the page now says why. pycortex derives each
    sulcus's label position from its path geometry at load (Shape.get_labelpos, one label per path,
    so a two-hemisphere sulcus is labelled twice for free) and writes data-ptidx itself in
    SVGOverlay.set_coords. data-ptidx is pycortex's output, not its input — a real overlays.svg
    contains zero <text> elements. v0.4.0 wrote <text data-ptidx="…"> labels with no x/y, and
    Labels.__init__ does an unguarded float(text.get('x')) over every <text> in a labels layer,
    so db.get_overlay(subject) raised TypeError before rendering anything.

Verification

Every API call in the new example was checked against this repo's source rather than recalled:

in the docs in the source
cortex.db.get_overlay('S1') cortex/database.py:349
svg.sulci SVGOverlay.__getattr__, cortex/svgoverlay.py:99
svg.sulci['CS'] Overlay.__getitem__, cortex/svgoverlay.py:338
make_figure(…, with_sulci=True) cortex/quickflat/view.py:37

The reStructuredText was parsed with docutils (Sphinx-only roles stripped) and is clean; I confirmed
that check can fail by feeding it a short section underline.

Sulcus drawing itself is unaffected — this is the documentation catching up with
pycortex-roidraw v0.4.1.

🤖 Generated with Claude Code

…ng sulci

The roidraw page (#656) said to "merge the fragment into the subject's
overlays.svg". Doing that literally means appending a second
<g inkscape:label="sulci"> layer -- and SVGOverlay.__init__ keys its layers by
inkscape:label:

    self.layers[layer.name] = layer

so the appended layer replaces the subject's own. Every sulcus already in that
file becomes invisible to pycortex, quickflat, and the WebGL viewer. The right
instruction is to copy the individual <g inkscape:label="..."> shape groups into
the subject's existing #sulci_shapes group, which this adds as a warning plus a
worked example.

Two other corrections, both from pycortex-roidraw v0.4.1:

- The export is no longer a bare fragment. It is a standalone SVG document that
  declares the svg and inkscape namespaces; the old fragment used the inkscape:
  prefix without declaring it, so no XML parser would open it.
- The exported sulci_labels layer is empty, and the page now says why: pycortex
  derives each sulcus's label position from its path geometry at load
  (Shape.get_labelpos, one per path), and writes data-ptidx itself via
  SVGOverlay.set_coords. A <text> element without x/y made Labels.__init__ raise
  TypeError on float(text.get('x')).

Docs-only. Every API call in the new example was checked against the source:
db.get_overlay(subject), SVGOverlay.__getattr__ -> svg.sulci,
Overlay.__getitem__ -> svg.sulci['CS'], and make_figure(..., with_sulci=True).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the documentation in docs/roidraw.rst to clarify that drawn sulci are exported as a standalone SVG rather than an SVG fragment. It adds a new section, "Installing drawn sulci into a subject," which provides step-by-step instructions, XML and Python examples, and a warning about correctly merging the exported groups into a subject's existing overlays.svg file. No review comments were provided, so there is no feedback to address.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

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.

1 participant