ENH add types and tests for quickflat - #682
Draft
kroq-gar78 wants to merge 5 commits into
Draft
Conversation
kroq-gar78
changed the title
kroq-gar78
force-pushed
the
types/06-quickflat-core
branch
from
August 20, 2026 22:25
80ddb08 to
28ac9d3
Compare
kroq-gar78
force-pushed
the
types/05-dataset-core
branch
from
August 21, 2026 00:23
defba84 to
eed5da5
Compare
mvdoc
force-pushed
the
types/06-quickflat-core
branch
from
August 21, 2026 00:37
28ac9d3 to
27b3bf1
Compare
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.
kroq-gar78
force-pushed
the
types/06-quickflat-core
branch
from
August 21, 2026 08:18
27b3bf1 to
999df01
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
1 participant
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 adds type support for the non-SVG functions in
cortex.quickflat, and it also improves test coverage for the many arguments ofquickflat.make_figure.One of the big changes this PR makes is that it eagerly imports
matplotlib-- it seems like this was avoided in the past, but maybe it's not much of an issue nowadays?(This separates the quickflat-specific code out from #670 .)
Stack created with GitHub Stacks CLI • Give Feedback 💬