Repository navigation
Merge main into Vnext_Update - #997
Merged
Merged
Conversation
writeSyncIndex: atomic write, checked fopen, no leaked handles
…name/email in every UID-taking method Two coordinated changes for the NDI Cloud demo, done in lockstep with Waltham-Data-Science/NDI-python#168. Filename and SecretsFilename now resolve to fullfile(userHome, '.ndi', ...) via a new userPrefDir() static, matching ndi-python's default. A one-time migrateLegacyIfNeeded() runs on load: if ~/.ndi/ has no profiles file but the old prefdir pair still exists, it is copied over so upgraders keep their saved profiles. ensurePrefDir() creates the directory on demand from saveToDisk and aesWriteSecret so the first write does not fail before anyone has interacted with the file. findIndex now accepts a UID (exact), a Nickname (exact), or an Email (case-insensitive), tried in that order; an ambiguous nickname/email raises NDI:cloud:profile:ambiguousProfile naming the candidate UIDs rather than silently picking one. setCurrent, setDefault, remove, and switchProfile always store the resolved UID rather than the caller's raw key so every downstream comparison stays UID-based. Test suite is updated accordingly: the prefdir-anchored filename test now checks that filename() lives under userPrefDir(); new tests cover lookup by nickname and email, the ambiguous-match error, and the "resolved UID is stored" invariant on setCurrent/switchProfile/remove. Refs #920, Waltham-Data-Science/NDI-python#168. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zDzZ9joBkZjzeMLfWZZk7
+cloud/profile: move preferences to ~/.ndi/ and let findIndex accept nickname/email/UID
….ndi/ Follow-up to #920 / Waltham-Data-Science/NDI-python#168. Both cross-language stores that already agreed on ~/.ndi/ (NDI_Preferences, NDI_Cloud_*) are now joined by the local sessiontable. PathConstants.Preferences retargets from fullfile(userpath, 'Preferences', 'NDI') to fullfile(userHome, '.ndi'), matching the location ndi-python writes to. The value is inlined here rather than delegated to ndi.cloud.profile.userPrefDir() because Constant property initializers cannot reliably call static methods across every supported MATLAB release. The `Todo: Use prefdir` comment is now obsolete and removed. sessiontable.localtablefilename() inherits the new location automatically; no other in-tree caller of PathConstants.Preferences exists. ndi.session.sessiontable.migrateLegacyIfNeeded runs on first getsessiontable(): if ~/.ndi/local_sessiontable.txt is absent and the old fullfile(userpath, 'Preferences', 'NDI', 'local_sessiontable.txt') exists, both it and its `_bkup*` siblings are copied over. Legacy files are left in place (copy, not move) so a downgrade to a pre-migration MATLAB keeps working -- same rule as ndi.cloud.profile.migrateLegacyIfNeeded. Failures warn under NDI:sessiontable:migrateFailed rather than raising. Coordinated with Waltham-Data-Science/NDI-python#172. Closes #922. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zDzZ9joBkZjzeMLfWZZk7
Add a comment on the existing copy-before-open in OldDatasetTest so the reason is visible where it matters: ndi.dataset.dir runs repairDatasetSessionInfo on a legacy dataset_session_info document during construction, which writes and deletes documents. Opening a shared example directly would mutate it. MATLAB has no equivalent test that opens a shared external corpus in place, so the actual fix for the sibling issue lands on the Python side (ndi-python#99). Cross-linking here so the pattern stays visible if MATLAB later grows such a test. Refs ndi-python#99. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BbK6Xis72nMM1EpBYEKsPK
Codecov flagged 22.22% patch coverage on #923 because the new migrateLegacyIfNeeded and its helpers are only reached when a legacy fullfile(userpath, 'Preferences', 'NDI', 'local_sessiontable.txt') exists, which never happens on a fresh CI runner. The existing ndi_test_sessiontable script exercises the API surface but not the migration, so codecov correctly measured the new lines as unexercised. Adds tests/+ndi/+unittest/+session/sessiontableMigrationTests, seven tests covering every branch: - PathConstants.Preferences resolves to ~/.ndi/ (regression guard). - localtablefilename() lands under PathConstants.Preferences. - migrateLegacyIfNeeded copies the legacy table into the new location and leaves the legacy in place. - Backup siblings (_bkup001.txt, _bkup002.txt) are copied too. - Migration is a no-op when the new file already exists (new wins). - Migration is a no-op when no legacy is present. - getsessiontable() triggers the migration end-to-end. TestClassSetup/Teardown snapshot and restore any pre-existing files at either location (a real developer's saved sessiontable) so the suite is safe to run locally, matching the pattern already used by profileTests. No changes to sessiontable.m -- the test is purely additive. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zDzZ9joBkZjzeMLfWZZk7
+common/PathConstants + +session/sessiontable: land preferences in ~/.ndi/
…w45q OldDatasetTest: explain why the corpus is copied before opening (parity note for ndi-python#99)
…opened the binarydoc (#509) Before this change ndi.dataset/database_closebinarydoc unconditionally called ndi_dataset_obj.session.database_closebinarydoc(...), i.e. the dataset's INTERNAL session, even for binarydocs the open path had routed to a linked/member session via open_session(session_id). That mismatch meant the wrong session's autoclose_listeners map was consulted and the wrong database driver was asked to close the handle, leaking the DID lock on the linked session's store. Fix: track (binarydoc key -> owning session) on the dataset itself. - New protected property BinaryDocSessions = containers.Map, keyed by the binarydoc's fullpathfilename (the same key the session's autoclose_listeners already uses). - database_openbinarydoc calls rememberBinaryDocSession(binarydoc, session) whenever it dispatches to a linked session. - database_closebinarydoc calls lookupBinaryDocSession first and dispatches to that session's close; falls back to the internal session when no owning session is recorded (matches prior behavior for internal-session docs and for handles opened outside this dispatch path). - forgetBinaryDocSession clears the entry so the map does not grow across a session's lifetime. The autoClose path was already correct: the SESSION's database_openbinarydoc registers the ObjectBeingDestroyed listener, so when a binarydoc handle is GC'd the owning session's callback fires. This fix is only about the explicit-close path. The Python side of this issue (dataset.database_openbinarydoc missing cross-session dispatch on the open path) was landed in the sibling repo as Waltham-Data-Science/NDI-python#175 / PR #176. Python file handles rely on GC/`with`, so there is no analogous close-mismatch to fix there. Test: tests/+ndi/+unittest/+dataset/datasetBinaryDocCloseTests.m, three cases: close through the dataset for a member-session doc is warning-free; the map entry is gone after close; and the recorded session for a linked-session binarydoc is the member session, not the internal one. MATLAB test suite has not been run in this session (no MATLAB installed). Reviewer should run `runtests('ndi.unittest.dataset.datasetBinaryDocCloseTests')` and the surrounding `ndi.unittest.dataset` package locally before merging. Closes #509. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zDzZ9joBkZjzeMLfWZZk7
… #11288) The initial commit set the property default to a containers.Map. A handle-typed property default in MATLAB is shared across every instance of the class, so two open ndi.dataset objects would have written into and read from the SAME owning-session map -- a close on one dataset could try to route through a session recorded by a different dataset. Code Analyzer flagged it and it is a real bug. Fix: default the property to [] and lazily allocate the map in a new ensureBinaryDocSessions helper called from rememberBinaryDocSession (and reachable from lookup/forget, which already tolerate a non-map value). Every dataset instance now gets its own map on first use. Test coverage from the previous commit is unchanged and remains valid; the map's per-instance behavior is exercised implicitly by the tests, which use a single freshly-constructed dataset per test. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zDzZ9joBkZjzeMLfWZZk7
+ndi/dataset: route database_closebinarydoc through the session that opened the binarydoc (#509)
Line 21 of apps/calculations/simple_calc_v2_schema.json was `"parameters": [-Inf,Inf,0]`. `Inf` / `-Inf` are MATLAB literals but not valid JSON tokens; strict JSON parsers (Python's json module in particular) reject the file. MATLAB's own jsonencode emits `Infinity` (not `Inf`) when `ConvertInfAndNaN=false`, and jsondecode only accepts `Infinity`, so `Infinity` works for both languages. The sibling v1 schema uses `Infinity` -- v2 was likely hand-edited to `Inf` thinking it was equivalent. The same file lives in ndi-python (synced from here in Waltham-Data-Science/NDI-python#177) and turned CI red on main after #178 landed a tripwire test that parses every bundled schema. Fix pushed in lockstep in ndi-python#(TBD) so the next sync stays clean. Five-character diff. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zDzZ9joBkZjzeMLfWZZk7
Sibling of the NDI-python change of the same name; completes the deferred symmetry item from Waltham-Data-Science/NDI-python#78 (b). PR #87 there explicitly noted no battery was added and "worth discussing separately whether an ensemble round trip belongs in it." This is that battery for filter, the one +ensemble function that is pure and in-memory. The other eight need a session, a clock, or a display and would turn the battery into a fixture-management exercise rather than a cross-language comparison. Fifteen cases pin every rule stated in the MATLAB help: includes are a UNION across names/ids/index/Keep, excludes always win over includes, IncludeIndex is 1-based on both sides, all-zero trailing columns are trimmed EXCEPT when nothing is kept -- MATLAB's isempty guard returns early and the 0-by-Smax width survives. That last one is the one a port is most likely to smooth over, so shapeOut is compared alongside activityOut and nothingKeptPreservesWidth pins it as 0-by-3 rather than 0-by-1. Activity is encoded row by row rather than as a 2-D matrix, per section 3 of FUN_CASES_SCHEMA.md: a 2-D matrix would render in MATLAB's column-major order against Python's row-major, which the schema rules out. Rows are 1-D, so the encoding follows parseText's inputRendered pattern. Files: tests/+ndi/+symmetry/+fun/ensembleFilterCases.m tests/+ndi/+symmetry/+makeArtifacts/+fun/ensembleFilter.m tests/+ndi/+symmetry/+readArtifacts/+fun/ensembleFilter.m tests/+ndi/+symmetry/FUN_CASES_SCHEMA.md (section 10) Every input field and every output field carries into the artifact, joined by case name. identifier and message are recorded but never compared, per the standing rule that MATLAB identifiers and Python exception names cannot match. /!\ AUTHORED WITHOUT A MATLAB RUNTIME -- the expected values are derived by reading src/ndi/+ndi/+fun/+ensemble/filter.m branch by branch. Every case class is documented and the standing prediction/measurement workflow (predict, defer, then settle) applies to any that turn out wrong on the first real run. Each generator carries its own /!\ marker as reminder. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XRaABckSNjs3e3bYN4FmHS
CI runs MATLAB, so the "authored without a runtime, validate before relying on it" warning is not useful to a reader looking at the merged file -- the symmetry workflow already exercises it. The predict/defer/settle workflow for a case that turns out wrong on first run is documented in FUN_CASES_SCHEMA.md sections 5 and 9 and applies whether the author had a runtime or not. Removes the markers from the three new files; no code changes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XRaABckSNjs3e3bYN4FmHS
simple_calc_v2_schema: use Infinity, not Inf (invalid JSON)
Symmetry battery for ndi.fun.ensemble.filter
The sparse code path is what production actually takes -- spike_matrix
returns sparse, and ndi.util.readSparse / writeSparse is the NDI-specific
round-trip format built exactly so an ensemble moves sparsely between the
two languages. Dropping the "keep the rendering symmetric" argument, since
both filter implementations densify their sparse output before rendering
and the resulting signature is identical to a dense twin's.
Two new cases:
sparseInputBasic mirrors includeNamesBasic
sparseNothingKeptPreservesWidth mirrors nothingKeptPreservesWidth
Case definitions grow a `storage` field ("dense" | "sparse"); the runner
wraps activity in a scipy csr_matrix before calling filter when it says
sparse. `input_signature` carries `storage`, so one language sparse and
the other dense would fail as a real disagreement rather than pass
because the numbers happened to match. The nothingKept twin is
deliberately included: the isempty guard in local_trim_columns has to
fire on a 0-row sparse slice too, and the pair is what pins that.
Also fixed _rendered_rows / _output_shape to densify a sparse matrix
before iterating (np.asarray on a CSR returns an object array
pointing at it, not a dense array); both `filter`s return sparse output
when handed sparse input.
Battery grows from 15 to 17 cases; make-artifact assertion updated.
Verification: 4 new tests pass (2 skip on matlabArtifacts as usual),
23 unit tests on ndi.fun.ensemble.filter still pass. Ruff/black clean.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XRaABckSNjs3e3bYN4FmHS
…nmk2 ensembleFilterCases: add sparse activity coverage
…cs-hdf5-format-9jn3l9 # Conflicts: # .github/badges/code_issues.svg
…mat-9jn3l9' into claude/spatial-genomics-hdf5-format-9jn3l9 # Conflicts: # .github/badges/code_issues.svg # .github/badges/tests.svg
A pyramid's ladder is split across two document types: the ladder, grid, origin and base pixel size live on spatialGeneExpressionPyramid, while each level's pixel dimensions, tile size and stored-tile count live on its own spatialGeneExpressionTiles document. Until now only a private helper inside readViewport reached across them, so anything else that wanted to set up a viewer, size a figure or pick a level had to re-derive the join. levelTable does that join once, returning one row per level, finest first, with the shared frame (origin, extent, base pixel size, index order) in UserData. Finest first because that is the order a multiscale viewer wants a ladder in. readViewportBase fixes the frame mismatch. readViewport takes its rectangle in the pixels of one level, zero-based and relative to that level's upper-left -- right for the tile arithmetic, wrong for a caller, who has a rectangle in the source coordinates the data was acquired in, the same frame cell centroids and contours use. The conversion subtracts the origin and divides by the bin size, and the rounding has to go outward or the caller silently gets less than it asked for. Low edge floors, high edge ceils, and info.rectSourceCovered reports exactly what came back, so the usual case of not landing on a bin edge is visible rather than assumed. Panning off the pyramid returns an empty image rather than throwing, because a viewer does that routinely. chooseLevel picks the coarsest level that still resolves a rectangle. Documented as NOT needed for napari, which takes the whole ladder and picks a level itself from the shape ratios; it is for the callers that render one image -- a figure, a thumbnail, an export, a tile server. Missing the target returns the finest level and says so rather than erroring, since asking for more detail than the data holds is a legitimate request with a definite answer. Tests use a non-zero origin and a wider-than-tall extent so an origin-blind or transposing implementation fails rather than coincidentally agrees, and they check the two things that go wrong quietly: the cross-document join, and the rounding at the edges. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHxsZ56GZsZ3hJ98TJ81t9
The spec names cells.tsv's columns cell_index, cell_id, x, y, area, dnb_count, total_counts, n_genes, but only the first four are required and writers differ on the rest. The extraction spike that produced the first opossum cells wrote 'dnbCount' and 'n_genes_by_counts' where the spec says 'dnb_count' and 'n_genes' -- so a reader that took columns by position would have mapped one measurement onto another's name and said nothing. Columns are therefore matched by header, and any column the spec does not name keeps the header the writer used rather than being renamed or dropped. cell_id stays text. It is a 14-digit identifier that loses precision as a double and then no longer matches the source file it came from. Contours are not read. contours.bin holds the boundary polygons and needs its own reader; info.contoursPresent says whether there is more, so a caller can tell the difference between "no contours" and "not read yet". A row count disagreeing with the document's n_cells warns rather than raising or being trusted silently: the file is what was read, and the mismatch is worth saying out loud. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHxsZ56GZsZ3hJ98TJ81t9
…mat-9jn3l9' into claude/spatial-genomics-hdf5-format-9jn3l9
The figure and its layouts were already on c.offWhite. The CONTROLS were not: a uitable, a uitextarea, a uilistbox and a uieditfield each default to MATLAB's own grey chrome, and the table is the largest surface in the window -- so the app read as a grey one with a tinted border rather than like the other NDI applets, which are white bodies under a navy header. A paper() helper beside accent() puts them on c.white with c.darkBlue text, and Browse... picks up the accent that every other button in the app already had. Defensive in the same way accent() is, and for the same reason: a missing handle, or a control with no BackgroundColor, is a no-op. Losing a window over a colour would be a poor trade. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHxsZ56GZsZ3hJ98TJ81t9
onView ended in uiwait(d), so pressing View froze the MATLAB prompt and
the manager window until the dialog closed -- while the whole point of
the dialog is to read a command and compare it against the list of
pyramids it came from.
Removing uiwait is safe because the dialog's callbacks are NESTED
functions: MATLAB keeps this workspace alive for as long as a handle to
one of them exists, so localGo, localRefresh and localBrowse still see d,
the fields and the selected row after onView has returned. The dialog's
default close request deletes it, which is what the old post-uiwait
cleanup line did by hand.
Two things follow from it becoming a real top-level window:
- PRESSING VIEW TWICE now raises the open dialog instead of building a
second one. Nothing stops a non-modal button being pressed again, and
two dialogs would leave the user editing whichever landed on top
while an older one still held a different pyramid.
- CLOSING THE MANAGER takes the dialog with it. An orphan whose Launch
button still worked would report its result to a status bar that no
longer exists. The napari process is deliberately NOT touched: it was
detached on purpose, it is somebody's picture, and closing the window
that launched it is not a request to close it.
`launched` goes with the wait. Its only reader was the post-uiwait
cleanup, so it was left assigned twice and read nowhere -- worth deleting
rather than silencing with a pragma.
The tests check the source rather than opening a window: CI has no
display, and what would actually go wrong here is somebody reinstating
the wait, which is visible in the text and not otherwise observable
headlessly.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YHxsZ56GZsZ3hJ98TJ81t9
The viewer is a separate process started with system(), so it inherits MATLAB's environment -- and NDI_CLOUD_TOKEN lives there. That is the whole of the viewer's cloud credentials: it cannot read the MATLAB Vault, and a login in some other shell does not reach it either. So the token MATLAB happens to be holding at the moment of the click is what the picture gets, for as long as the window stays open. Left alone that expires, and what it looks like from the viewer is not a login prompt but MISSING DATA. Tiles already in the local cache keep drawing; the ones that were never fetched fail one at a time as the user pans -- "Binary file 'tile.bin_280' not found for document ..." -- which reads as a broken pyramid rather than a lapsed token. Blurring makes it worse, since map_overlap gives each visible tile its eight neighbours and so reaches tiles a still view never asked for. freshenCloudToken renews it just before the launch, and says so plainly if it cannot. NO TOKEN AT ALL IS NOT AN ERROR and does not prompt: its absence says this MATLAB has not been talking to the cloud, and a login dialog thrown up by a button that says "View in napari" would be a surprise. Only an expired token is acted on, with interaction off -- the vault and the environment are read, nothing is asked. A token that cannot be decoded is left to the server to judge, which is isTokenExpired's own choice. The decision is split into cloudTokenNeedsRenewing so it can be tested: the renewal itself reads the vault, whose contents differ on every machine. Six tests, including one that pins the refresh AHEAD of the system() call -- refreshing after it would hand the child the stale one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHxsZ56GZsZ3hJ98TJ81t9
…ommand line Six laminar markers, in cortical depth order so the layers come up reading down the cortex rather than in an order that has to be explained: HPCAL1 green, RORB blue, FEZF2 bop orange, TSHZ2 yellow, NXPH4 red, SST cyan. The colours run along the spectrum with depth, which makes the stack legible without a legend, and all six are distinct because the layers blend additively -- two genes in one colormap make a picture that is neither of them. 'bop orange' is a napari colormap, not a typo; napari ships bop blue, bop orange and bop purple. The space in it is why the whole value is shell-quoted as one argument: unquoted it would reach the viewer as two, and the second would be read as a session path. THEY ARRIVE BESIDE THE BASE LAYER, NOT INSTEAD OF IT. That is the difference between the new --gene-layers and the existing --genes: --genes filters the base image down to a subset and leaves one picture, while this adds a layer per gene on top, so "All genes" stays and the six are comparable against it. A test pins that the demo command carries the one and not the other. THE PRESET LIVES IN MATLAB. The viewer takes genes and colours as ordinary command-line options, so a demo is a preset command rather than a mode the viewer has to know about -- anyone can build the same picture by hand, and a second demo needs no Python change at all. Nine tests, all headless: the preset's contents and the distinctness of its colours, the spec builder including a gene left without a colour, and the four things viewCommand has to get right. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHxsZ56GZsZ3hJ98TJ81t9
…at-9jn3l9 GEF Manager, an HDF5 table of contents, and one-based tile files
The navigator's Apps menu opens the manager unconditionally, so a second click on the same session used to build a second top-level window against the same database; both listed the same pyramids and only one carried the state the user was working in (last-selected row, a Reload the user just did). The View dialog inside the manager already handled this for itself; the manager window now does too. The tag is set on the figure and embeds session.id(), so findOpenFigure uses graphics as its own registry -- a closed window disappears from the search on its own. Different sessions still get their own windows. Fixes #984. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B1HUSKgeD1VfzgjK5sSAh1
GEF Manager: raise the open window for a session instead of duplicating it
DID-matlab#201 (merged as PR #202) opened the read path for a series whose manifest bytes are only remote: seriesMemberPath now offers the manifest's file_info location to the customFileHandler, caches the result at filecachepath/<manifestUid>, and proceeds. That path had no NDI-side coverage: FileSeriesRoundTripTest requires NDI Cloud credentials and only covers SyncFiles=true (the manifest arrives with its bytes), and TestDownloadedSeriesManifestKeepsItsCloudLocation checks only the file_info shape without touching the database. Add TestSeriesWithCloudOnlyManifestResolvesThroughHandler: an offline suite that builds a demoNDISeries with ndi.document.addFileSeries, reshapes its file_info into the SyncFiles=false shape via ndi.cloud.sync.internal.updateFileInfoForRemoteFiles, adds the doc to a DID sqlite database, and opens a member with a uid-keyed mock handler. Four tests: * The handler is called for the manifest first (ctx.uid = manifest's own uid, seriesName='', mode='open', sourcePath = the ndic:// URL verbatim), and the member reads back byte-for-byte. * The manifest lands at filecachepath/<manifestUid> so the next cached_path_for_uid on that uid is a hit. * Three different member opens share ONE manifest handler call, the 28,000-member-level guarantee. * A handler that serves nothing for the manifest still raises DID:SQLITEDB:open with the "not on this machine" message shape -- the identifier and phrase callers already pattern-match on. The test uses did.implementations.sqlitedb directly rather than ndi.dataset.database_openbinarydoc: NDI's didsqlite hard-codes @download_file_from_cloud at open time (right at runtime, wrong here), and ndi.session.database is not publicly reachable to swap. The document is still built through ndi.document / addFileSeries and reshaped through updateFileInfoForRemoteFiles, so the NDI-side pieces this test exists to cover are still exercised. Deferred to a follow-up on #986: retiring the cloud-reference workaround in updateFileInfoForLocalFiles.m:96-101. That change needs verification against the existing FileSeriesRoundTripTest, which requires NDI Cloud credentials.
Adds testMembersSurviveADownloadFromTheCloudWithSyncFilesFalse to FileSeriesRoundTripTest. Companion to testMembersSurviveADownloadFromTheCloud above (SyncFiles=true), reusing the same upload/dataset lifecycle. The new test downloads with SyncFiles=false -- so no bytes are installed locally and the manifest arrives as an ndicloud/ingest=0 file_info entry only -- and opens each member end-to-end. VH-Lab/DID-matlab#201 (merged as PR #202) opened the read path for a cloud-only manifest: DID's seriesMemberPath now offers the manifest's file_info location to the customFileHandler, caches the result at filecachepath/<manifestUid>, and proceeds. That call shape had no cloud coverage before now: every existing round-trip test downloads the manifest bytes at SyncFiles=true and only asks the handler for members, so the real ndi.cloud handler's manifest-fetch branch had never been exercised in CI. The offline half of #986 is pinned in ndi.unittest.TestSeriesWithCloudOnlyManifestResolvesThroughHandler (#987, offline mock handler); this one exercises the actual download_file_from_cloud. What the test pins beyond byte equality: * Precondition -- neither the manifest nor any member is on this machine before the first open. Without this a run that silently installed the manifest would still open the members and say nothing about the path. * After the first open, the manifest lands at filecachepath/<manifestUid>, so subsequent member opens on the same series pay no network for the manifest again. Every open in the loop but the first depends on this. * batch presign uidMisses == 0, i.e. every uid (the manifest's included) answered from the batch. signerCalls is not tightly asserted -- SyncFiles=false may reasonably cost one call (a whole-document scope answers both) or two (manifest then series scope), and pinning one over the other would guess at a server behaviour this suite does not otherwise cover. Deliberately not used: the ndi.test.lightsheet.makeBlobFixture on the in-development lightsheet-zarr-ndi-viewer branch. Basing this PR on an unmerged branch would either block on that branch or duplicate the fixture, and the question this test needs to answer is narrower than a lightsheet ingest -- byte-for-byte comparison against demoNDISeries content proves it. A lightsheet-based end-to-end test is a natural follow-up once that branch lands.
… helper
Two of the four new tests failed on CI because they read back
`calls{1}` as empty even though the member reads succeeded --
proving the handler had run but the recorded calls list was not
what the caller saw.
Root cause: the previous `uidKeyedHandler` helper defined the mock
as a nested function inside the helper and returned `callsHandle`
as an output. MATLAB nested functions share the enclosing
function's workspace, so writes went into the helper's own
`callsHandle` variable -- but a cell array returned from a
function is a value copy. The caller's `calls` was a snapshot
taken at return time and never updated afterwards. Every call to
the handler kept updating the helper's captured workspace, which
the test could not read.
The fix mirrors DID-matlab's own convention (see
TestSeriesMemberFetch.testHandlerGetsTheSeriesNameAndTheMemberUid):
define the mock as a nested function inside each test method that
needs to inspect `calls`. The closure then writes into the test
method's own workspace, which the assertions read directly. No
helper return-by-value, no lost updates.
testTheManifestLandsAtFilecachepath and testDifferentMembersShareOneManifestFetch
also inline their handlers for consistency, and the unused
uidKeyedHandler helper is removed.
updateFileInfoForRemoteFiles now reconstructs `files.series_info.ingest_locations` for any series with n_present > 0, in the same shape SyncFiles=true produces via reconstructSeriesIngestLocations. This closes the DID-matlab #185 MembersNotLocatable guard that was refusing every SyncFiles=false download of a dataset carrying a series document (the failing test in FileSeriesRoundTripTest.testMembersSurviveADownloadFromTheCloudWithSyncFilesFalse was hitting exactly that shape). The manifest bytes are pulled to a scratch directory just long enough to read each present slot's uid, then discarded. Manifests do NOT land in the DID file cache here; the first member open triggers DID-matlab #201's fetchSeriesManifestBytes which is what installs the manifest at filecachepath/<uid>. If the cache is evicted on a later session, the same handler-fetch path re-populates it. Nothing on the caller depends on a persistent local manifest file. Fetches go through batchSignedUrlLookup (with the document scope), then fall back to per-uid getFileDetails, mirroring didsqlite's own download_file_from_cloud handler. A manifest that cannot be fetched leaves the entry un-reconstructed so DID#185's guard raises with the document's own identity on the following add_docs -- the right voice for a partial download. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015dKpDXCo8VTS4AHiibNnCp
- Drop the %#ok<NASGU> on the onCleanup binding: current MATLAB no longer flags the assignment, so the suppression is stale (Code Analyzer 11382). - Split the two-line `try, rmdir(d,'s'); catch, end` into explicit lines so the comma flagged as unnecessary at line 204 (Code Analyzer 11383) is gone. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015dKpDXCo8VTS4AHiibNnCp
updateFileInfoForRemoteFiles now accepts an optional customFileHandler name-value pair. When provided, the manifest fetch goes through the same DID contract (destPath, sourcePath, context) that DID#201's fetchSeriesManifestBytes uses on the read side, so a caller can hand in one handler that serves both the download-time reconstruction and the later member opens. The default (no handler passed) still mints a signed URL and fetches directly via ndi.cloud.api.files.*, matching what didsqlite.m's own download_file_from_cloud does for a non-member ndic:// file. No caller in the tree needs to change -- downloadNdiDocuments continues to call this without a handler and gets the same behavior as before. Offline test coverage: tests/+ndi/+unittest/+database/TestUpdateFileInfoForRemoteFilesShape covers the shape work and the reconstruction path without cloud credentials. The reconstruction test rounds a demoNDISeries doc through did.implementations.sqlitedb so its ingest_locations are stripped (matching a cloud read-back), then reshapes with a mock handler that serves the manifest bytes by uid -- the same handler shape TestSeriesWithCloudOnlyManifestResolvesThroughHandler uses on the read side. Asserts that ingest_locations comes back with one entry per present member, ndic:// URLs against the given dataset, and that the manifest fetch happens exactly once with seriesName='' in the context (a manifest, not a member). The cloud-CI FileSeriesRoundTripTest still exercises the default (no-handler) path against a real NDI Cloud dataset. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015dKpDXCo8VTS4AHiibNnCp
…986) - makePlainDocWithOneFile referenced 'plain.bin' on a demoNDISeries doc. The schema only declares 'chunkdata.bin', so add_file raised "file must match an expected name". Switched the plain-doc fixture to the demoNDI class (schema declares one plain file, no series), which is the shape this test wants to exercise anyway. - testReconstructsIngestLocationsThroughAMockHandler passed the doc returned by did.implementations.sqlitedb/get_docs straight into updateFileInfoForRemoteFiles. get_docs returns a did.document; the SUT is an NDI helper that calls has_files (an ndi.document method), so the call raised NoSuchMethodOrField. Wrap the returned doc in ndi.document(...) the same way didsqlite.do_read does. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015dKpDXCo8VTS4AHiibNnCp
…false-test Offline test for a cloud-only series manifest (SyncFiles=false shape) (#986)
…lse-companion Cloud companion: SyncFiles=false round-trip in FileSeriesRoundTripTest (#986)
MATLAB wrappers for the doc-level file-tier control endpoints landing on
the ndi-cloud-node side (Waltham-Data-Science/ndi-cloud-node PR for
manuals/file-tier-design.md).
New under src/ndi/+ndi/+cloud/+api/+files/:
- setFileTier(cloudDatasetID, documentIDs, targetTier, ...)
POST /datasets/{datasetId}/file-tier-jobs. documentIDs is a string
array or cell array; each entry may be a cloud _id or an NDI id and
the server infers by shape unless idNamespace= is set explicitly.
targetTier is one of STANDARD | STANDARD_IA | GLACIER_IR | GLACIER |
DEEP_ARCHIVE.
- getFileTierJob(jobID)
GET /file-tier-jobs/{jobId}. Returns state, targetTier, per-phase
counts, and a bounded error list.
- waitForFileTierJob(jobId, ...)
Polls with exponential backoff until the job reaches a terminal
state (completed | failed | superseded) or the timeout elapses.
Same shape as waitForSignedURLSetJob.
Plus the implementation classes under +implementation/+files/ and two
new entries in +api/url.m (create_file_tier_job, get_file_tier_job).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012XnjPsVgQfRTSJRu2AeABL
ndi.cloud.download.internal.batchSignedUrlLookup collapsed two distinct causes of "this uid is not in the scope's map" into one silent per-uid fallback: 1. Data drift: the file was never in this scope. The right answer is to fall back and warn. 2. A lagged index: the file IS in the scope on the server, but the batch endpoint's map has not caught up with a just-completed bulk upload. Falling back per uid here defeats the batch design -- a 28,000-member series that took the fallback would issue 28,000 getFileDetails calls where the batch would have taken one. Waltham-Data-Science/NDI-python#309 (the counterpart on the Python side) observed the second case on TEST_USER_1/prod after Waltham-Data-Science/NDI-python#308 first let a series manifest reach the server: the batch answered signerCalls=1 with a 2-entry map for a 4-member series. The two missing uids fell back per uid. Retrying the same scope 37 minutes later on run_attempt=2 returned the full 4-entry map -- so the endpoint does eventually catch up, just not by the time waitForAllBulkUploads reported done. Fix (#991): when a cached scope answers with a URL but a subsequent uid in that scope misses, drop the entry, pause for partialMapRetrySeconds (default 0.5), and re-fetch once. If the second map covers the uid, take it and every subsequent uid in the scope resolves from the refreshed cache; if not, warn and fall back as before. Bounded to one retry per scope, so a genuine data-drift miss still costs one extra signer call for the whole scope, not one per uid. Tracked in a new STATS.partialMapRetries counter so tests can distinguish the retry path from a plain data-drift miss. Refactor: the fetch/nargout/try dance moves into a nested localFetchScope so the retry can call it a second time without duplicating it. Adds an injectable sleepFcn (defaults to @Pause) so tests exercising the retry path do not pay real wall-clock latency, and a partialMapRetrySeconds=0 mode that keeps the single retry but skips the sleep. The live-cloud FileSeriesRoundTripTest.signerCalls assertion is relaxed to signerCalls == 1 + partialMapRetries with partialMapRetries <= 1, keeping the "one batch call per series' worth of members" contract while allowing the retry. uidMisses == 0 stays firm. Four new unit tests in BatchSignedUrlLookupTest mirror the Python counterpart's TestBatchLookupPartialMapRetry: the retry succeeds; the retry pauses before re-fetching; the retry is bounded to once per scope; a retry that raises still falls back cleanly without double-counting the miss. Existing miss-path tests are updated to account for the added signer call and to skip the sleep via partialMapRetrySeconds=0. Closes #991. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016rEFEXaZ9t3BftShqc9Y4B
…etry Retry once when the batch signer returns a partial map
readchannels_epochsamples_ingested addresses concrete _seg.nbf_N files. In NDI the first sample of an epoch is 1, so s0 <= 0 must be clamped rather than passed through: unclamped it computed SEG_start = 0 and asked the document for _seg.nbf_0, a filename no writer has ever produced. The isinf-only block already handled -Inf that way; a finite value before the epoch (or past the end) fell through unclamped. pyraview's filter warm-up sets t_read_start = current_t - excess on the first chunk, which epochtimes2samples converts to s0 = 1 - sr, a large negative sample number. Over local files the daq system dispatches to a reader that hands the number to an external file library, which quietly returns a short slice, so the same shape has always worked. Over ingested files the reader tries to name a specific segment document, DID rightly refuses "does not include a file named ai_group1_seg.nbf_0", and pyraview's document-creation try/catch swallows the error and never creates the pyraview doc. The fix extends the reader's own convention: read what's in [absolute_beginning, absolute_end]; anything outside is silently omitted. -Inf/+Inf now flow through the same max/min the finite bounds use (MATLAB clamps them for free). A window entirely outside the epoch returns zeros(0, numel(channel)) rather than throwing -- read past the edge, get nothing there. The caller-inversion check (s0 > s1 originally) is kept, and precedes the clamp so an inverted request is not silently absorbed by clipping one endpoint into the other. The tests ingest the buildSession Intan example in place, then read the resulting probe through readtimeseries -- pyraview's own call chain -- with the windows that used to fail: before t0, after t1, entirely before t0, and [-Inf, Inf]. Each asserts on the reader's advertised contract; before this change the first two error inside DID. Fixes #993 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011auYUP2oxabnq8Z2q7rv6F
The four new tests errored on CI with MATLAB:cellRefFromNonCell inside
isingested. Root cause: probe.getchanneldevinfo returns devepoch as a
cell of epoch IDs (strings) -- see src/ndi/+ndi/probe.m:212-214 --
not the epochfiles cell array that isingested wants.
isingested does epochfiles{1} on its input, which is fine for a cell of
paths and MATLAB:cellRefFromNonCell for a plain string.
Do what the reader itself does at src/ndi/+ndi/+daq/+system/mfdaq.m:178:
load the daq system by name, ask its filenavigator for the epoch's
files, then test isingested on those. That is the exact cell array the
dispatch decision uses, so the guard proves what it claims.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011auYUP2oxabnq8Z2q7rv6F
waitForFileTierJob was a monolithic function -- no seam to override -- so the terminal-state, timeout and backoff paths could only be exercised against a live server (which for tier control is not deployed to dev yet, per PR #990's test plan). This mirrors the shape of the sibling waitForSignedURLSetJob (thin wrapper over an implementation class with an overridable pollStatus seam, plus a Scripted subclass and a mock test) so those paths run headless. - Factor the wait loop into a new WaitForFileTierJob implementation class with a protected pollStatus() seam. - waitForFileTierJob.m becomes a thin shim over the class, unchanged API. Small behaviour fix along the way: an ok=false poll no longer ends the loop -- a gateway blip is not a dead job, and the caller keeps polling until the deadline. The old function returned false on the first API error, which a live test would only hit on a bad day. - ScriptedFileTierJob under tests/+ndi/+test/+helper/ subclasses the new impl class and scripts pollStatus from a states cell array, same shape as ScriptedSignedURLSetJob. - FileTierMockTest under tests/+ndi/+unittest/ exercises six cases against the helper: completed => success, failed and superseded are terminal (no re-poll), queued -> running -> completed is polled through, running-forever times out, and a transient ok=false is retried rather than treated as terminal. A live cloud test belongs under tests/+ndi/+unittest/+cloud/ (matching SignedURLSetTest) and lands once the tier endpoint is deployed to dev. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012XnjPsVgQfRTSJRu2AeABL
Clamp the ingested mfdaq reader's window to what the epoch has
Mirrors SignedURLSetTest under tests/+ndi/+unittest/+cloud/. Two tests: - testCreateAndWaitForFileTierJob: builds a small local dataset with two generic_file docs, uploads to a fresh cloud dataset, waits for bulk extraction to settle, then submits setFileTier against one doc with targetTier='PSEUDO_COLD'. PSEUDO_COLD skips S3 entirely and is server-side-accepted in non-production stages, so the round-trip is fast and pays no S3 minimum-storage charges. Verifies the accepted payload (jobId, fileCount, resolvedDocumentCount, collateralDocumentIds), polls once (accepting completed as a legal first read because v1 runs the worker inline), waits for terminal state, and checks state=='completed' with filesDone==fileCount and filesFailed==0. On a partial deployment (CONFIGURATION_ERROR / ENQUEUE_ERROR / JOB_CREATION_ERROR / INVALID_TARGET_TIER) the test skips rather than fails, matching the sibling. - testGetFileTierJobUnknownId: bogus jobId returns 404. Follows the narrative + APIMessage pattern so a failure carries every step and the URL of the last API call. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012XnjPsVgQfRTSJRu2AeABL
The fourth MATLAB wrapper called for by manuals/file-tier-design.md (ndi-cloud-node PR #138). Reads the doc's cached filesTier summary -- the same field the tier worker rewrites on completion -- and normalizes it into a stable shape the caller can rely on: answer.counts per-tier file-count map answer.dominant coldest tier with a non-zero count (the design's 'dominantFileTier' shorthand); "" for a doc that has never had a tier op answer.notes cell array of divergence notes ('warmest-wins overruled a freeze because sibling doc needs file warm'); empty when clean answer.updatedAt timestamp of the last summary write answer.raw the full get-document response, for callers that want the whole doc No new server endpoint -- the design deliberately keeps the summary as a cached field on the document, so this composes over the existing ndi.cloud.api.documents.getDocument and projects out filesTier. Callers that already hold the doc can just read `.filesTier` themselves; this wrapper is for the common 'give me the tier state for this doc, no setup' path. Two new cases in FileTierTest under +cloud/: - testGetFileTierOnUntieredDocumentReturnsEmpty -- read on a fresh doc round-trips cleanly and reports dominant "" (or STANDARD, depending on how the server surfaces the never-touched state) rather than throwing on the absent field. - Extension to testCreateAndWaitForFileTierJob -- after the PSEUDO_COLD freeze completes, getFileTier.dominant reads "PSEUDO_COLD". This is the read path a caller uses to confirm a freeze happened, without polling the job. Both live under +unittest/+cloud/ so they skip cleanly when NDI_CLOUD_USERNAME / NDI_CLOUD_PASSWORD are absent, matching the sibling SignedURLSetTest pattern. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012XnjPsVgQfRTSJRu2AeABL
…-o8enda feat(cloud.api): file-tier control wrappers
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Analyzer found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Contributor
Author
|
Merge landed directly on
Closing this PR — it was preview-only. Generated 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
5 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.
Bring
mainforward into theVnext_Updatestaging branch so v2 work can pick up the fixes that have landed onmainsince the last sync (2026-08-13).Since the last sync
mainhas moved from the state merged intoVnext_Updateon 2026-08-13 (3442d4f) to6fdec7e(2026-09-16). Notable landings onmain:setFileTier,getFileTier,getFileTierJob,waitForFileTierJob, plusScriptedFileTierJobhelper andFileTierMockTest.pyraviewfilter warm-up path that asked DID for_seg.nbf_0; window is now clamped to the epoch's absolute range.getFileDetails.customFileHandler(paired with DID-matlab added clear button #202) — unblocks retiring theupdateFileInfoForRemoteFilesworkaround.Known conflict shape
The prior
Vnext_Updatemerge (69dd7da) resolved conflicts insrc/ndi/+ndi/validate.mand the badge SVGs.validate.mwas removed on the Vnext line long ago, so any conflict on it should be resolved as deleted on our side wins. Badges regenerate; keep whichever, they'll be overwritten by the badge workflow anyway.Not to merge as-is
This PR is for reviewing the diff before we do the merge locally. Once we're happy, the actual sync will be a
git merge origin/mainonVnext_Updatewith the conflict resolutions above, then push. We may close this PR without merging.Generated by Claude Code