Open
Conversation
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
kaitj
force-pushed
the
maint/reorg
branch
3 times, most recently
from
4f1ed15 to
003e126
Compare
Combination of manual and agentic asissted fixes for tests with new organization
graph/methods/fetchers.py — deleted
GraphFetchers was a pure passthrough onto GraphCache with identical signatures. Its filtered list methods (fetch_surface_atlases, etc.) were moved directly onto GraphCache as get_surface_atlases, get_surface_transforms, get_volume_atlases, get_volume_transforms.
graph/methods/cache.py — merged + extended
Absorbed all fetcher methods (renamed fetch_* → get_* for consistency with the existing single-item getters), and gained require_surface_atlas — the raising variant that both ops classes previously duplicated privately.
Moved to graph/cache.py and deleted graph/methods
graph/models.py — simplified
Removed the ABC base and abstractmethod decorator. All four subclasses implemented fetch() identically as return self.file_path, so the method now lives once on the concrete Resource base.
graph/builder.py — de-duplicated
Four _parse_* methods (two for atlases, two for transforms) had the same nested-iteration shape. Collapsed to _parse_surface_resources and _parse_volume_resources, each accepting the target model class and a fixed_fields dict. The name prefix logic is derived from fixed_fields uniformly.
graph/utils.py — fetchers → cache
GraphUtils previously held a fetchers: GraphFetchers reference. Now holds cache: GraphCache directly and calls cache.get_surface_atlases / cache.get_surface_transforms.
graph/transforms/surface.py — fetchers → cache, _require_surface_atlas removed
SurfaceTransformOps dropped the fetchers field and the duplicated _require_surface_atlas. All lookups now go through self.cache.get_* or self.cache.require_surface_atlas.
graph/transforms/volume.py — same treatment
VolumeTransformOps dropped fetchers and its own copy of _require_surface_atlas. The _project_volume_to_surface ribbon dict comprehension replaced the sequential for-loop over ("white", "pial").
graph/core.py — fetchers removed, data_dir typed
self.fetchers is gone. The data_dir walrus-operator pattern gives it a clean Path | None type at construction. All fetch_* public methods now call self._cache.get_* directly.
Tests — fetchers → cache, fetch_* → get_*/require_* throughout. The two _require_surface_atlas tests moved from test_surf_to_surf.py to a new TestGraphCacheRequire class in test_graph.py. The vol-to-surf tests that simulated a miss via return_value=None were updated to use side_effect=ValueError(...) since require_surface_atlas raises rather than returning None. _Unit tests co-written and optimized by by Claude Code_
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 is a massive overhaul to make
neuromaps_prime.graphmore maintainable. It does move some things around, but breaks things into smaller modules.Test coverage is a little lower now, but will look into it as I add tests to cover additional methods.😰