Fix audit findings in the deprecated paths, mocks, CI, and README - #65
Open
rly wants to merge 1 commit into
Open
Fix audit findings in the deprecated paths, mocks, CI, and README#65rly wants to merge 1 commit into
rly wants to merge 1 commit into
Conversation
PoseEstimation built from the deprecated `nodes` and `edges` arguments holds a Skeleton that is reachable only from that object, so the link has no target in the NWBFile and the object fails to write with an OrphanContainerBuildError. Raise at construction with an explanation instead. Files written before ndx-pose 0.2.0 store nodes and edges on the PoseEstimation group, so construct mode keeps building the Skeleton and those files read as before. An empty `devices` list carries no device, so it no longer raises a DeprecationWarning. The default pose_estimation_series of mock_PoseEstimation hold (x, y) positions in pixel space, matching the single camera view a PoseEstimation object represents. The per-camera views in the MultiCameraPoseEstimation roundtrip test hold pixel-space positions for the same reason. The example usage tests run the examples with sys.executable rather than whatever `python` resolves to on PATH, from the repository root rather than the working directory, and remove the NWB file even when an example fails. The Windows wheel installation test sources the venv activation script. Running `activate.bat` under bash leaves the venv inactive, so the step installed into and imported from the outer environment. The version comes from pyproject.toml, so checkout no longer fetches the full history for tags. `*.nwb` in .gitignore is negated for the back-compat test fixtures, which are tracked and would otherwise need `git add -f`. `license` is the SPDX expression BSD-3-Clause (PEP 639) with LICENSE.txt in `license-files`. README: repair the mismatched quote in the mermaid init directive of both diagrams, the stray angle bracket on PoseTraining, the version attribute name on MultiCameraPoseEstimation, the SkeletonInstance-to-Skeleton arrow, and the missing labeled_video relationship. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
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
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.
Motivation
A repo-wide Claude Code audit turned up a broken deprecated code path, three CI/test defects, and a set of documentation and packaging errors. This fixes the ones that affect correctness or are cheap and unambiguous.
Correctness
PoseEstimationbuilt from the deprecatednodesandedgesarguments could not be written. TheSkeletonconstructed from them is reachable only from thatPoseEstimationobject, so the link has no target in the NWBFile andio.write()fails withOrphanContainerBuildError. The constructor now raises with an explanation of what to build instead. Construct mode is unchanged, so files written before ndx-pose 0.2.0, which storenodesandedgeson thePoseEstimationgroup, still read as before.Making the old path work would require a schema change (a
Skeletonchild insidePoseEstimation, which the spec does not allow and which conflicts with keeping skeletons in aSkeletonscontainer), so this converts a confusing failure at write time into an actionable one at construction. It is listed under Breaking changes in the CHANGELOG.An empty
deviceslist no longer raises aDeprecationWarning. An empty list carries no device, so it is not a use of the deprecated behavior.Tests and CI
test_example_usage.pyruns the examples withsys.executablerather than whateverpythonresolves to onPATH, from the repository root rather than the working directory, and removes the NWB file even when an example fails. The tests now pass when pytest is invoked from an unrelated directory, which they did not before.activate.batunderbashleaves the venv inactive, so the step was installing into and importing from the outer environment, where ndx-pose was already present, and passed unconditionally.pyproject.toml; there is no VCS-based versioning.Fixtures
The default
pose_estimation_seriesofmock_PoseEstimationhold (x, y) positions in pixel space, matching the single camera view aPoseEstimationobject represents. They previously held (x, y, z) data labelledpixels. The per-camera views in theMultiCameraPoseEstimationroundtrip test are updated for the same reason.Packaging and docs
licenseis the SPDX expressionBSD-3-Clause(PEP 639), withLICENSE.txtinlicense-files. Verified in the built wheel:License-Expression: BSD-3-Clause,License-File: LICENSE.txt, and the file is bundled underdist-info/licenses/..gitignorenegates*.nwbforsrc/pynwb/tests/back_compat/. Those fixtures are tracked, so a new one would otherwise needgit add -f.PoseEstimation's class docstring describes the single camera view scoping, matching the schema and README."primaryBorderColor') that made it invalid;PoseTraininghad a stray third angle bracket;MultiCameraPoseEstimationusedsource_software_versionwherePoseEstimationused thedataset__attributeform;SkeletonInstancepointed atSkeletonwith the containment arrow despite being a link; and thelabeled_videolink had no relationship line.Testing
pytest src/pynwb/tests: 60 passed, 309 subtests passedruff check src/ examples/: passesnodes/edgesraises and that0.1.1_poseestimation_nodes_edges.nwbstill reads back its skeleton nodes with no unexpected warnings.Not included
The audit also flagged items left alone here: the
nodes/edgesentries in__nwbfields__being inert, the setters raisingValueErrorrather thanAttributeError, black and ruff both being configured with neither enforced in CI,pre-commitin the dev group with no config, the stalesrc/matnwbsdist exclusion, the empty[tool.pytest.ini_options], and the unedited ndx-template placeholders indocs/source/.🤖 Generated with Claude Code