Repository navigation
Conversation
…nd also fixes to catalog run display
… chunk awareness and protection against poorly-chunked Tiled data.
…r logic was moved fully into view layer.
… whole GUI on bad Y data
…f in-memory Zarr to new L1. Still problems with redraws flashing screen
Steps 1 and 2 moved every file in the package, so open steps citing fetch/plan.py or geometry/bundle.py were sending a reader to paths that no longer exist. Open steps are re-pointed and re-measured at 211886d. Line numbers moved with the import rewrite, so they are re-read rather than adjusted: plan_fetch is 113-208 rather than 115-211, the .fetch calls in trace.py are at 230 and 267, Trace.get_plot_bundle at 213, the trace_key calls at 75 and 159, and single_canvas.py's call at 398. run/pipeline.py is 450 code lines rather than 455. Everything cited outside models/plot was unaffected. Done steps keep the paths they were taken against, since that is what they describe. The convention is stated at the head of the Steps section so it does not have to be rediscovered each time: an open step must send a reader to a path that exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Step 3 of the description chain plan, in one commit because the two halves are one edge reversal: removing reduce_cached_plane from stages.py is what deletes `stages -> request`, and request.py gaining materialize_view is what adds the opposite. Split apart, the first commit is a cycle. _plane_axis_arrays becomes PlotBundle.axis_arrays() and loses its frame argument entirely. Every field it read off that frame is on the bundle -- the shape is y.shape, the render mode is the bundle's own, and the two plot dims are the constants view_frame supplies -- so the frame was never an argument, only the habit of a function with no object to live on. It also open-coded view_frame's axis-name padding, which is now one _padded_names() helper. reduce_cached_plane becomes PlotRequest.reduce_cached_plane. Everything it reads -- plane axes, profile axis, spatial reduce, region, mask mode -- is the request's own, and it was the only thing tying stages.py to the chain above it. build_plot_bundle takes is_roi_profile: bool instead of a whole PlotRequest it consulted at exactly one line. tests/test_bundle.py had been building a PlotRequest and a ViewIntent to supply that one boolean; the helper is gone. spec/plan.py is now the only module in spec/ that imports PlotRequest, which is the precondition step 4 needs. stages.py drops 75 code lines. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Six free functions become methods, and the last edge inside spec/ that ran upwards turns round with them. plan_fetch becomes PlotRequest.plan. Its body moves file: spec/plan.py no longer imports spec/request.py, and is now FetchPlan, narrow and kept_axes, importing nothing from its own package. The planner sits with the description it reads. build_plot_bundle, prepare_1d_bundle and prepare_2d_bundle become PlotBundle.pack, .from_1d and .from_2d -- three constructors for the type they construct, named for what they take rather than for what they do. compile_with_mask_mode becomes RegionDefinition.compile_masked, a concrete base-class method: every subclass answers it identically once it has said how to compile, and inverting is a property of the mask, not of any one shape. copy_plot_bundle and _storage_axes_from_bundle leave run/frozen_spectrum.py for PlotBundle.copy and PlotBundle.storage_axes, which makes that file about frozen spectra only. storage_axes is public, not the private method the plan called for, because its caller is in another module. It sits beside axis_arrays and says why the two are different questions and not one written twice: storage order against display order, mesh edges against centres, and 1-D as well as 2-D. copy() now keeps row_reversed and col_reversed. The free function dropped them, so every copied bundle claimed no axis had been reversed; no test noticed because a frozen bundle is re-read through storage_axes and get_data, neither of which reads the flags. Also removes a duplicate block of three type aliases and an orphaned comment fragment that step 1's extraction left in plane/roles.py. 527 tests pass; no runtime cycles; lint unchanged at 107. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The maintainer rejected the KeyReader half of the split: a non-caching reader reaching back to its parent RunSource for every load and key operation is artificial, and is the no-forwarding-models rule -- "which keys exist" and "read this key" are one concept with two verbs. Measuring RunFetch per method settled the rest. It reaches into exactly five RunSource methods; the cache half needs only four of them, and 170 of the cache's 289 lines reach for none at all, being pure window arithmetic over plans and held blocks. So the reader surface already exists and does not need moving to be found. The inheritance in the old shape was also the real obstacle to the testability the split was for. CachingReader(KeyReader) cannot be constructed without a real reader and therefore a real CatalogRun, which would have left the cache less testable than it is today. A cache holding its reader can be handed a stub, as ChunkCache already is in tests/test_chunk_cache_l2.py. Step 5a is now: extract BlockCache into run/cache.py, composed over a reader, and give the window logic its first direct tests. No key access moves, so the 11 production call sites outside run/ are untouched. The cost is recorded rather than hidden: RunSource does not shrink, and the post-refactor review's 250-method-line target for it is declined, not missed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BlockCache moves to run/cache.py, taking whatever reads the run's keys as a
constructor argument. The contract is documented there and is four methods
wide -- load, read, describe, plot_axis_names -- which is narrow enough to
stub, and 170 of the class's lines use none of them.
RunSource constructs one and answers block(plan). It is the reader for its
own key space, so there is no reader class to wrap: the cache is a
collaborator the run owns and hands its own reads to. No key-access method
moves, so the call sites outside run/ are untouched.
RunFetch keeps the sequencer and falls from 215 code lines to 113; _load_block
is now three lines, planning the fetch and asking the source for the block.
It survives until 5b so the 76 test sites churn once.
tests/test_block_cache.py is eleven tests against a stub reader that counts
its reads, modelled on how test_chunk_cache_l2.py tests the data-layer cache.
The containment logic had no direct coverage before: a repeat, a contained
narrowing checked against the same window of the whole array, an open-ended
held slice, three ways of reaching outside the held block, a moved integer
index, a different key, clear(), and both norm paths.
One thing removed rather than moved. get_plot_bundle's debug line reported
cached={self._block is not None}, evaluated after the load had already
populated the cache on a miss -- so it was always True. Hit/miss telemetry
belongs on the cache if anyone wants it.
538 tests pass, 11 of them new; no runtime cycles; lint unchanged at 107.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PlotRequest.plot_bundle(reader) is the end of the chain and the only place the reader enters it. The stages between a load and a pack read nine attributes of the request and one of the plan, so they belong with them; on RunFetch they were a class whose whole state was a back-reference. run/pipeline.py is deleted, and with it RunFetch, RunSource.fetch and the last mutual pair in the package -- run.pipeline <-> run.source. Nothing in run/ describes a plot any more: it holds the reader, the block it caches, and the frozen spectra. _plane_frame comes along as a private PlotRequest method. _render_hint does not: the sequencer already calls describe(ykey) for the synthetic check, so the hint comes off that same KeyInfo and the second call disappears. 75 call sites moved, all but two of them in tests. The rewrite was done by AST rather than by pattern, because the receiver and the first argument swap places -- source.fetch.get_plot_bundle(request, **kw) becomes request.plot_bundle(source, **kw) -- and many of the calls span lines. tests/test_stages.py's load-boundary helper reached through the old private _load_block. It now makes the two public calls the sequencer makes, which is what it was always testing. Trace.get_plot_bundle is examined and kept. The names read distinctly at their call sites, and its real defect -- a get_ prefix on a method that mutates last_bundle and the render mode -- is older than this plan and belongs with the rest of Trace in step 6. 538 tests pass; no runtime cycles and no mutual pairs left in the package; lint unchanged at 107. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
6a stays the structural move: trace/ as a package, TraceKey.of, and PlotRequest.trace_key deleted. 6b is the rename 5b examined and deferred -- Trace.get_plot_bundle to Trace.fetch -- kept separate because a three-file move and a 31-site rename do not review well in one commit. The rename earns itself: get_ promises an accessor and the method writes last_bundle, _fetched_request and the render mode, which is the very thing preview_roi_profile exists to avoid. And the name is not invented for the occasion. needs_fetch() is already the predicate for it, so the class ends up stating its cache of one in a single vocabulary, and 5b freed the word by deleting RunSource.fetch and RunFetch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
It is the last thing in the package carrying the word, and it is internal: PlotRequest.plan returns it and RunSource.block consumes it, neither of them on a surface a caller meets. The defect was two exposed things called fetch one directory apart, not the root appearing twice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
trace/ holds key.py, trace.py and set.py. TraceKey moves out of spec/request.py, and PlotRequest.trace_key becomes TraceKey.of -- the arrow was backwards, because a description package cannot name a thing in the package above it. spec/ now names nothing above itself, and request.py is the top of the chain rather than a module that also knew about traces. trace.py and trace_set.py move under it as trace.py and set.py, following the convention the rest of the re-carving used: no re-export surface, and consumers name the module they want. 538 tests pass; no runtime cycles; lint unchanged at 107. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The get_ prefix promised an accessor for a method that writes last_bundle, the fetched-request fingerprint and the render mode -- which is exactly what preview_roi_profile exists to avoid doing, as its own docstring said. The name was already in the class. needs_fetch() is the predicate for this method and gates the call that runs it, so Trace now states its cache of one -- last_bundle, needs_fetch, invalidate_bundle, fetch -- in one vocabulary. Step 5b freed the word by deleting RunSource.fetch and RunFetch. The plot_request parameter becomes request, redundant once the method says fetch, and passed by keyword at exactly one site. One thing the rename exposed: eight test names still carried get_plot_bundle from before 5b, when it meant RunSource.fetch.get_plot_bundle. They exercise PlotRequest.plot_bundle now, so renaming them along with the method would have pointed them one altitude off. They are test_plot_bundle_* instead. 538 tests pass; lint unchanged at 107. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Step 6a was committed with `git add -A`, which swept in three untracked things that had been sitting in the working tree: .gitattributes, catalog_config.toml and test/. None of them had anything to do with giving Trace its own package. That commit was amended to drop them and the branch replayed, so the two hashes the progress table recorded no longer exist. Steps 1 through 5b are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…chain
Answers planDocuments/load_pipeline_problem_statement.md, which found a chain
of descriptions of one plot at increasing concreteness -- Projection,
PlotRequest, FetchPlan, PlotBundle -- spread across three packages that each
held half a ladder and half a vocabulary, and so imported each other.
The eight steps are recorded in planDocuments/plot_description_chain_plan.md
with the commit each landed in. In outline:
plane/ the vocabulary a plot plane is written in. Imports nothing in the
package, which is what killed the view <-> geometry cycle.
spec/ the chain itself, importing strictly one way and ending at
request.py, the only module that knows the whole of it.
run/ the reader, the one block it caches, and frozen spectra. Nothing
here describes a plot any more.
trace/ the long-lived objects a session keeps, and the key that names one.
Free functions went onto their types: PlotRequest.plan, PlotBundle.pack /
.from_1d / .from_2d / .copy / .storage_axes, RegionDefinition.compile_masked,
TraceKey.of. RunFetch, run/pipeline.py, RunSource.fetch, view/, geometry/ and
fetch/ are all deleted.
Two decisions were taken against the plan as drafted and are recorded in it.
RunSource is the reader, so no KeyReader was introduced and only the cache
was extracted, composed rather than inherited -- inheritance was what would
have made the cache untestable. And FetchPlan keeps its name, being internal.
Out of scope and still open: the block cache's key, capacity and location;
reduce_masked_plane, which has no production callers; B905 across
models/plot; and the whole-tree ruff format pass.
538 tests pass, 11 of them new; no runtime cycles; lint unchanged at 107.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pixi.lock is machine-written and enormous, so a three-way merge on it produces a conflict nobody can resolve by reading. merge=binary makes git take one side and leave regeneration to pixi, and the linguist attributes keep it out of the repository's language statistics and collapsed in diffs. The file was already in the working tree untracked. It belongs in the repository: it is a rule about how this project is handled, not a local preference. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
catalog_config.toml holds one machine's catalog list, tiled URLs included, and test/ is an ad-hoc scratch directory -- a kafka config, a catalog exercise script, and editor backups. Neither is part of the project, and both are easy to sweep into a commit by accident, which is how they briefly ended up in a refactor commit on the branch this merges. Note that test/ is not tests/: the suite is unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A workflow that installs the pixi environment and runs `pixi run test`, on every push and on pull requests from forks. Same environment and same command as locally, so a green run here means what it says. The suite needs a Qt platform plugin and CI has no display, so the variable moves onto a pixi task rather than being typed at the command line: `pixi run test` sets QT_QPA_PLATFORM=offscreen itself. Nobody has to remember it, and the workflow runs the same command a developer does. The conda-forge Qt is self-contained -- libGL, libEGL, libxkbcommon and the offscreen plugin all come from the environment -- so no system packages are installed. Two things are deliberate and commented in the file, because both look like oversights: `locked: true` is not set. The lock records this package installed editable, whose setuptools-scm version comes from `git describe`, so the lock is stale one commit after it is written and the check would fail on every run. The job skips the pull_request event for same-repository pull requests, which the push event on the branch already covers. Otherwise every commit on an open PR runs the suite twice. Lint is not a job here. `ruff check` currently reports 107 findings, all of them pre-existing, so a lint gate would be red from its first run. It belongs in this workflow once that backlog is cleared. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The "Show <name>" entries were built in a loop, and each one's handler read the loop variable when the user clicked it rather than when it was built. By then the loop had finished, so with two or more columns hidden every entry in the menu un-hid the same one -- whichever was hidden last -- and the other names did nothing. Binding col as a keyword-only default fixes it. Keyword-only because QAction.triggered passes a checked bool positionally, which would otherwise land in the bound parameter. Found by ruff's B023, which is why that rule is in the selection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A leftover print("In KafkaViewerTab.py") sat above the imports, which is
where all ten of this tree's E402 findings came from: every import in the
file was, correctly, not at the top of it.
It also wrote to stdout as a side effect of importing the module, which is
the reason the rule exists -- code that runs at import time before the
imports have run is code nobody meant to write.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Eleven of the unused-import findings were in these two __init__ files, where the imports exist so that a catalog can write `from ..data import KafkaRun` and the source palette can pull seven models in one line. An __all__ states that, which is both what the rule asks for and the honest answer: the names are the package's surface, not leftovers. Both files gained a docstring saying so, and noting that this is the opposite of the convention under models/plot, where no package re-exports and the import site names the module deliberately. The difference is real -- there the module name tells you which rung of the chain you are on; here the module name is an implementation detail of which file a run class sits in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Thirty-three of them, all legacy: Qt widgets left behind when a layout was rewritten, typing names that outlived their annotations, an asteval Interpreter and a `time` that moved elsewhere during the plot refactor. Applied with `ruff check --select F401 --fix`, then checked by importing every touched module in a fresh interpreter, because most of these files are views that the suite does not construct and a removal it got wrong would otherwise surface only at runtime. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The two that were real: BlueskyRun.getRunKeys starts a second timer, t2, before the descriptor lookup, and then reports that lookup's duration as time.time() - t1 -- the first clock. Every debug line for the descriptor step has been reporting the dimension-hints step as well. Using t2 is what the unused variable was for. test_create_from_config_unknown_type asserted False after the call it expected to raise. Under `python -O` that assert vanishes and the test passes whether or not the ValueError arrives. It is pytest.raises now, which also checks the message the old version checked. The rest are cosmetic: six loop variables nothing reads get a leading underscore, five f-strings with no placeholders lose the prefix, and the two catalog sources that re-raise a load failure now say `from e` so the original error is attributed as the cause rather than appearing as an error during handling. One removal: a test helper built a RunListItemModel and bound it to a local nothing used. The adapter takes no Qt parent, so its only reference was that local and it was collected as soon as the helper returned -- it never reached a test body. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Thirty-two calls, and the question is per-site: does a length mismatch mean
a bug, or is the shorter side the point? Twenty-eight say strict=True and
four say strict=False with a comment giving the reason.
Most of the strict ones are already guarded. tile_indices validates
len(slice_info) == len(shape) and len(shape) == len(tile_chunks) at the top
of the functions that then zip them; run/cache.py returns early when the two
slice tuples differ in length; RunSource._plot_names raises before zipping
its names against the key's dimensions. In those, strict=True restates an
invariant at the point of use and can never fire. The rest zip things built
together -- names against roles, dims against shape, metadata against data.
The four lenient ones are real:
catalog/table.py a chunk near the end of the catalog returns fewer rows
than the range asked for.
spec/stages.py the second operand comes back from a user-written
transform and may be anything; the body already checks
each entry's shape.
controls/dimension associated arrays and their names both default to empty,
and a slider with neither should build, not raise.
data/base.py a source reporting the wrong number of dimension names
is a genuine failure, but KeyInfo.of is where it is
diagnosed and its message names the key and the rank.
This one was caught by the suite: strict=True here
replaced that error with zip's own.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The workflow ran only the suite, because ruff reported 107 findings when it was written and a lint job would have been red from its first run. The cleanup took that to zero, so the gate goes in. It is a separate job rather than a second step, so a lint finding and a failing test are separate signals on a pull request instead of one hiding the other -- steps are sequential, and whichever ran first would stop the job. The last four findings were in nbs_viewer/io/export_to_xdi.py: two bare excepts, now naming Exception, and two functions sharing a mutable default dict across calls, now None. See the note below about that file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Member
Author
|
Added tests + lint to CI. Merging to master, and then I'll test, tag, release. |
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.
The big Refactor:
My goodness, what have we done here. Let's lay down at least a sketch of where we started, and where we are now.
Current Situation
We are now in a place where the code base is much larger and more complex than it was 6 months ago. But it is more comprehensible. Too much thoughtless LLM use had resulted in a situation where it was actually impossible to reason about the code without assistance, and that has now been fixed. The code is now encapsulated enough that you can reason about small chunks, and we can go forward in a more clean way.