Draw an inventory: its path, its layout, and its epochs - #952
Conversation
…e map path() no longer draws the CRS position axes as line panels; columns= names the non-position geometry columns worth a panel, and tracks= selects which lanes are drawn and in what order. Inventory.viz hosts path, map and timeline.
distance= and time= replace distance_limits/time_limits, matching the coordinate names selection already uses; either end may be None. The timeline leaves out epochs which fall outside its window rather than clipping them to a sliver at the edge. Adds tests for the renderer and the plots, and documents them in the inventory tutorial, the visualization page, and the tunnel recipe.
Six fixes from review. A numeric coloring left unstated fiber as NaN, which matplotlib draws transparent, so a placed cable vanished where it stated no value. It now takes the grey categorical coloring already uses, with a legend entry beside the colorbar. Numeric lanes are normalized per lane, but shared one colorbar, so two lanes were described by one scale only one of them had. Each numeric lane now carries its own labelled bar. The map rebuilt its categorical palette per path, so one value could be two colors in one figure and the legend named only the first. The palette is now the figure's. The timeline kept an epoch which only touched its window, against the half-open semantics epochs are stored with, and plot_lanes advertised datetime bounds but left ordinal ticks on the axis. Selecting a path said an address would disambiguate epochs, which only a time does, and an ambiguous acquisition key leaked InvalidInventoryError out of Inventory.resolve.
|
Warning Review limit reached
Next review available in: 17 minutes Limit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8222a81b61
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| candidates = found | ||
| if time is not None: | ||
| candidates = [x for x in found if x[3].is_effective_at(time)] |
There was a problem hiding this comment.
When a Network or FiberArray has multiple epochs and its child paths leave their bounds unset, those child bounds defer to the container (_containment_errors explicitly defines this). Filtering only with path.is_effective_at(time) therefore retains paths from inactive container epochs, causing path(..., time=...) to remain ambiguous or select invalid metadata. The same path-only filtering is repeated in map_path's no-selector branch, where obsolete and current layouts can be drawn together; filter the network and array epochs as Inventory.resolve does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3c610f5. Reproduced first: two networks with successive epochs whose child paths leave their bounds unset made path(time=...) report ambiguity, i.e. a time passed to disambiguate could not. _effective_at now intersects the network, array and path epochs, and map_path's no-selector branch uses the same helper (it needed _iter_paths' network, which it had been discarding). test_containers_decide_which_epoch pins it; its mutant — filtering on the child alone — fails.
| "start": one.start_time, | ||
| "end": one.end_time, |
There was a problem hiding this comment.
For checked inventories where a path or acquisition omits a time bound under a bounded FiberArray or Network, the omitted bound inherits the container's bound rather than remaining infinite. Copying only the child's times here makes timeline() hatch the row as unbounded and show it outside the period in which resolution can reach it; compute each displayed interval from the intersection of the network, array, and child epochs. The acquisition rows below have the same problem.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3c610f5. _effective_epoch intersects the network, array and child bounds, so a row is drawn over the period resolution can actually reach it, and an inherited bound is no longer hatched as unbounded. The same test covers the timeline half: a path with unset bounds under bounded containers used to make timeline() claim no epoch stated a time at all.
| items = [x for x in one.labels if x.group == color] | ||
| keys = [x.value for x in items] | ||
| if not items: | ||
| groups = tuple(dict.fromkeys(x.group for x in one.labels)) | ||
| msg = ( | ||
| f"color={color!r} names neither optical distance, a geometry " | ||
| f"column ({one.geometry_columns()}), a label group ({groups}), " | ||
| "nor 'coupling'." | ||
| ) | ||
| raise ParameterError(msg) |
There was a problem hiding this comment.
When map_path() draws multiple placed paths and color names a geometry column or label group present on only some of them, processing the first path without that track raises here and aborts the entire map. Partial coverage within a path is already rendered as gray “not stated” fiber, so an entirely unstated path should receive the same treatment; otherwise the documented default of drawing every placed path cannot be combined with ordinary heterogeneous inventory metadata.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3c610f5. The color name is now validated once against every drawn path, so a typo still raises (with the names the inventory does state), while a path that simply says nothing under a real name draws as "not stated" — the same grey and the same legend entry partial coverage within a path already used. test_a_path_without_the_color_is_unstated covers it.
| try: | ||
| context = inventory.resolve(acquisition_key, time) | ||
| except InvalidInventoryError as error: | ||
| msg = ( | ||
| f"Acquisition key {acquisition_key!r} names more than one " | ||
| "acquisition, which happens where it was reconfigured. Pass a " | ||
| "time as well, to say which of its epochs to draw." | ||
| ) | ||
| raise ParameterError(msg) from error |
There was a problem hiding this comment.
If acquisition_key is malformed, unknown, resolves to zero networks or acquisitions, or fails at another hierarchy level, Inventory.resolve() raises InvalidInventoryError, but this blanket handler always reports that the key names multiple acquisition epochs and asks for a time. For typos and missing entries, supplying a time cannot help and hides the actual diagnostic from resolve; only translate the specific ambiguity case and preserve or accurately wrap the other resolution failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3c610f5. The handler now keeps what resolve said — "resolves to 0 acquisitions" stays that — and only appends the time hint when no time was given, as advice rather than a diagnosis. test_unknown_acquisition_key_keeps_its_error asserts the preserved count.
| def path( | ||
| inventory, | ||
| optical_path=None, | ||
| *, | ||
| acquisition_key: str | None = None, | ||
| time=None, |
There was a problem hiding this comment.
The newly exported path, map_path, and timeline APIs leave core parameters such as inventory, optical_path, time, and color untyped even though the repository requires type hints on public functions. Complete these signatures so callers and generated API documentation expose the accepted inventory and selector types consistently. .agents/agents.mdL36-L40
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3c610f5. inventory, optical_path, time, color and aspect are annotated on all three public functions, with Inventory/OpticalPath/timeable_types imported under TYPE_CHECKING so dascore.viz stays lazily importable (pinned by tests/test_imports.py::test_matplotlib_not_imported).
Correctness. A path or acquisition which states no time bound defers to its network and fiber array, so selection and the timeline now intersect the three epochs rather than reading the child alone; a time passed to path() could otherwise fail to disambiguate the epochs it was passed to choose between. Resolution failures keep what resolve said instead of being retold as an ambiguity no time can fix. A map colored by a group only some paths state draws the others as unstated rather than refusing the figure. The timeline's window takes a missing end, refuses a reversed one, and refuses a bound which is not a time; the distance window is resolved before a figure exists, so a refusal leaks none. Geometry is sampled at every gap, so a gap shorter than the sample spacing still breaks the line rather than being drawn across. The renderer. Labels are measured rather than counted, so the same figure keeps them at any dpi. Gaps are asked of the data, not of the margin drawn around it. A color which names no colormap is a color. The palette is the path's, so drawing some tracks colors them as drawing all of them does, and lanes with pinned colors no longer spend palette slots. The figures. Components take an Okabe-Ito set, categories are drawn from tab20's dark half first so neighbours are not two shades of one hue, in-box labels carry a halo, a wide map gets a horizontal colorbar, and the constrained layout keeps furniture on the canvas. Tests. Two regression tests could not fail: the gap test read only segment endpoints, where a bridge has none, and the palette test used two paths whose labels were byte-identical. Both are rewritten against cases which distinguish, and every fix here is pinned by a test whose mutant dies.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #952 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 195 197 +2
Lines 24891 25715 +824
==========================================
+ Hits 24891 25715 +824
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Importing dascore.viz registers the namespace, so every existing test would pass with the pyproject entry point deleted; one test now reads the declaration itself. The palette constants are read through the module rather than bound at import, so rebinding one moves the lanes and the column panels together, which is what a module-level palette is for.
lane_gaps and the gaps= drawing branch had no caller here: they were built for the spool coverage plot, which is its own change. Deriving what a frame does not cover is not a plotting question either, so when it returns it belongs beside the other interval helpers in dascore/utils/intervals.py rather than in the renderer. pack= stays, since the inventory relies on it to keep overlapping intervals from drawing on top of each other.
|
✅ Documentation built: |
A handful of distinct numbers is a set of categories which happen to be numbered, so it now takes one color each and a bar which reads at the values themselves: three boreholes are three blocks, not a ramp through one and a half. More than a handful is a quantity, and still ramps. The map's scale also covers only what the projection draws. Seen from above a borehole is a point, so it was spending three quarters of the colormap on fiber with no visible length, leaving the trench in one narrow band. An arrow marks values carried past the end of the scale. Fiber nobody described reads n/a rather than 'not stated', and the halo which keeps a label off its own fill is thinner.
What these plots look likeEverything below is the tunnel deployment from
|
1 participant
Description
An
Inventorydescribes the observing system behind a fiber archive, and until now it could not draw itself at all:dascore/viz/held five patch functions and nothing else. The hand-drawndocs/_static/tunnel_deployment.svgin the tunnel recipe exists because the library could not produce it.This adds three plots on a new
Inventory.viznamespace, each looking along one coordinate:inventory.viz.pathtracks=picks the lanes (channels,components,coupling, label-group names);columns=draws a geometry column as a panel beneath;distance=(low, high)windows the axisinventory.viz.mapx=/y=pick the CRS axes;color=takes distance, a geometry column, a label group, orcouplinginventory.viz.timelinekind=andcolor=;time=(start, end)windows the axisThe window arguments are named for the coordinate they select on, the way
selectalready spells it, and either end may beNone.The renderer is the load-bearing part
The three plots sit on
dascore/viz/_lanes.py, a dataframe-first interval-lane renderer whose column names are parameters rather than a fixed schema. That is what letsspool.get_contents()be passed straight in later without a rename, and it is why the module is general from day one though it has one caller today.plot_lanesbuilds its rectangles into onePatchCollectionper lane per sub-row, so per-interval assertions still work throughget_paths()while a large frame stays a handful of artists.lane_gapsis the gap derivation on its own, since "where are the holes" is worth asking without a figure attached.Colour follows
value_kind— the same functionOpticalPath._check_label_groupsuses, so the picture cannot disagree with the validator. Strings are categorical and assigned frame-wide, so one value is one colour everywhere in a figure; numbers are continuous, normalized per lane; booleans take the lane's colour withFalseat low alpha.Choices worth a second pair of eyes
mapdrops any segment touching a NaN position, so the tunnel's slack coil reads as the break it is.distance_mapdraws as a tick. It states an origin, and the inventory records no channel count, so inventing an extent would be a lie in the one place users trust the picture.pathdoes not trim by default. A 1.5 km lead-in takes the figure unlessdistance=is given; honest by default, and the window is one argument away.mapwith no path named draws every path which places itself, whilepathandtimelineinsist on one. A map of one cable in an inventory of several would be the strange default.Later, out of scope here: spool availability and gaps, and
AnnotationSetviz. Both are consumers of the same renderer.Follows #938 (the example inventories these plots are tested and documented against) and #937 (which made
Inventorya namespace host).Review
A cross-model sweep (Codex on the CLI, Codex on this PR, and six Claude reviewers running blind to each other) returned 26 correctness findings; all are fixed and pinned by tests. The ones worth knowing about:
timepassed to disambiguate epochs fail to do so, and drew inherited bounds as unbounded.resolvefailure was retold as an ambiguity no time could fix; an address was offered as a way to choose among epochs of one path, which only a time does.Two of the regression tests written in the first round could not fail — the gap test read only segment endpoints, where a bridge has none, and the palette test used two paths whose labels were byte-identical. Both were rewritten against cases that distinguish, and every fix here was mutation-tested: the fix is reverted, and the test must fail.
The figures also had a styling pass: components take an Okabe-Ito set, categories are drawn from tab20's dark half first so neighbours are not two shades of one hue, in-box labels carry a halo so they survive a dark fill, wide maps get a horizontal colorbar, and constrained layout keeps the furniture on the canvas.
Changelog
Inventory.viz.pathplots an optical path's components, coupling, channels, and label groups against optical distance.Inventory.viz.mapplots where an inventory's fiber physically goes, breaking the line where no position is stated.Inventory.viz.timelineplots when each acquisition and optical path was valid.Checklist
I have:
docs/contributing/general_guidelines.qmd).I have (if applicable):