docs(quickflat): add missing Returns, fix stale kwargs docs in view.py - #692
Open
AgarwalNilay wants to merge 7 commits into
Open
docs(quickflat): add missing Returns, fix stale kwargs docs in view.py#692AgarwalNilay wants to merge 7 commits into
AgarwalNilay wants to merge 7 commits into
Conversation
Types add_curvature, add_data, add_hatch, add_colorbar, add_colorbar_2d, add_connected_vertices, and all of view.py/utils.py's non-SVG helpers. add_rois, add_sulci, add_custom, add_cutout, and _convert_svg_kwargs are deliberately left untyped here — they call into cortex.svgoverlay directly and land with that module in PR 8 instead (see PR_SPLIT_PLAN.md). Built by diffing this worktree's pre-typing composite.py/utils.py against types-easy's final versions and keeping only the non-SVG-function hunks, rather than replaying the ~14 original historical commits individually — several of those commits interleave SVG and non-SVG hunks in ways that don't split cleanly commit-by-commit. test_quickflat.py's four new ROI-specific test functions (test_roi_styling_parameters, test_roi_list_and_sulci_list, test_combined_parameters, and the ROI/sulci assertions inside test_display_flags) are included here rather than held back for PR 8, since they don't block anything at runtime and splitting test_display_flags mid-function is not worthwhile. Verified: zero diff against types-easy for composite.py (excluding the 4 SVG functions + the Sequence import, which stay untyped pending PR 8), and zero diff for utils.py/view.py/test_quickflat.py.
GitHub Actions failed this test with: Failed: Timeout (>240.0s) from pytest-timeout inside db.get_shared_voxels()'s A* search (cortex/utils.py's get_shared_voxels/shortest_path), called from add_connected_vertices. Not a regression from this PR's typing -- add_connected_vertices's own docstring already documents this path as "graphically intensive ... takes quite a while on some systems". It's simply the first test to exercise with_connected_vertices=True, and get_shared_voxels caches its expensive one-time result to filestore/db/<subject>/cache/shared_vertices_*.npy, which doesn't exist yet on a clean CI checkout. Reproduced locally with a fully cleared cache (rm filestore/db/S1/cache/shared_vertices_*.npy): 245.85s, just over pytest.ini's default 240s suite-wide timeout. That default exists so "a single hung headless browser session does not consume the entire CI budget", with individual tests expected to override via @pytest.mark.timeout(N) -- using that documented mechanism here rather than skipping or weakening the test.
get_flatcache calls utils.get_mapper() and reads mapper.masks, which currently type-checks only because get_mapper is still unannotated (Mapper's own typing PR hasn't landed yet, and now lands after this one instead of before it). Narrow explicitly with isinstance rather than resting on that coincidence, so this PR doesn't implicitly depend on the order Mapper's typing PR lands in. TODO: once Mapper is typed and get_mapper's return annotation makes this redundant, remove the isinstance assert and the Mapper import.
Registers a "slow" marker and excludes it via -m "not slow" in addopts, so this graphically-intensive test (per its own docstring) no longer runs on every default invocation. Drops the @pytest.mark.timeout(600) override that had been added to cover its cold-cache runtime: now that it's excluded by default, the override is unnecessary noise -- anyone running it explicitly with -m slow on a clean cache falls back to pytest.ini's suite-wide 240s timeout instead.
Removing it was a real regression, not just cleanup: this test still needs longer than pytest.ini's 240s suite default on a cold cache (~4-5 min), and -m slow lets it be run explicitly. Restored, with a comment noting it overrides the global timeout.
Contributor
|
Can you rebase onto this branch? |
make_figure had Parameters but no Returns section despite returning fig. make_png's docstring had drifted from make_figure's (duplicate `sampler` entry, and several params like `thick`/`depth`/`with_rois` group listed as if they were make_png's own args rather than kwargs forwarded to make_figure) -- reorganized under a documented **kwargs entry with each forwarded argument still individually described. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
AgarwalNilay
force-pushed
the
docs/quickflat-view-docstrings
branch
from
August 22, 2026 01:31
936defc to
25e8d96
Compare
…arlier Changed 'duration' to 'frame_duration' to correctly reflect function argument in docstring.
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
2 participants
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.
make_figure had Parameters but no Returns section despite returning fig. make_png's docstring had drifted from make_figure's (duplicate
samplerentry, and several params likethick/depth/with_roisgroup listed as if they were make_png's own args rather than kwargs forwarded to make_figure) -- reorganized under a documented **kwargs entry with each forwarded argument still individually described.