fix: restore pvplus.py, fix pyvista API drift, drop rvcprint from figure scripts - #44
Open
petercorke wants to merge 3 commits into
Open
fix: restore pvplus.py, fix pyvista API drift, drop rvcprint from figure scripts#44petercorke wants to merge 3 commits into
petercorke wants to merge 3 commits into
Conversation
…ure scripts Fixes #20. tools/pvplus.py was a symlink to a path on the reporter's old machine (/Users/corkep/Dropbox/.../frames.py) that never existed in this repo or anywhere accessible -- every figures/**/*.py script that did `import pvplus` was broken for anyone but the original author, and only by cwd accident even then. Recovered the file's content (three near-identical copies existed, all but one line identical) and placed it at RVC3/tools/pvplus.py instead of bare tools/ or figures/3d/: it's imported by ~40 scripts spread across figures/3d/ and multiple figures/code/chapterN/ subdirectories, and RVC3.tools is already a real, declared, pip-installable package -- the only placement that resolves for every consumer via a normal `pip install -e .` dev setup, with no PYTHONPATH tweaking needed. Swapped `import pvplus` -> `from RVC3.tools import pvplus` in all 40 consuming scripts (mechanical, one line each -- every pvplus.xxx(...) call site is unchanged since the bound name doesn't change). Also fixed outfile()'s hardcoded personal absolute path to derive from the calling script's own location -- same class of bug as the original issue, no reason to reintroduce it. Testing this surfaced two more real, pre-existing issues: - pyvista's PolyData.transform() now requires an explicit inplace= kwarg (used to default to True) -- added it at all 7 call sites in pvplus.py so it actually runs against a current pyvista install. Several individual figure scripts have their own local .transform() calls with the same issue; left those alone, out of scope here. - Several scripts import a book-production image-export helper (rvcprint) that's either equally broken (bare `import rvcprint` / `from rvcprint import outfile`) or just unnecessary for what we're testing here. Commented it out (import + the screenshot-path usage) in the ~10 pvplus-consuming scripts that referenced it, per discussion -- it's book-production plumbing, not needed to exercise the actual figure-generation logic. Adds a `figures` optional-dependency extra (pyvista, vtk, pillow) to pyproject.toml, since none of this was previously declared anywhere -- `pip install -e ".[figures]"` is now the documented way to get set up for regenerating book figures. ## Test plan - All 40 modified scripts pass `py_compile` - Ran 9 of the pvplus-consuming scripts end-to-end (add_frame, ribbon, axis, add_ellipsoid, ellipsoid_3d, add_frame2 all exercised), all exit 0 - 3 more (fig2_15.py, fig2_16.py, fig9_17.py in figures/code/) open a real on-screen render window since they don't set off_screen=True -- confirmed rendering correctly by eye, not run to completion headlessly
Correction to the previous commit, which touched rvcprint in some figures/code/chapterN/ scripts too. figures/code/README.md documents that rvcprint is the required, intentional mechanism those scripts use to save the book's PDF figures -- it was never broken there and shouldn't have been touched. Reverted those 6 files back to their original state (only the pvplus import fix remains). For the 6 figures/3d/*.py scripts that reference rvcprint (the tree issue #20 is actually about): none of them use it for their real output mechanism (they all call plotter.export_gltf(...) directly, confirmed by figures/3d/README.md), so removed the now-fully-unused import from each rather than leaving a disabled trace of it.
rvcpython -> rvc3python (doesn't exist on PyPI; rvc3python does, and matches pyproject.toml's declared name).
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.
Summary
Fixes #20.
tools/pvplus.pywas a symlink to a path on the reporter's old machine that never existed in this repo — everyfigures/**/*.pyscript doingimport pvpluswas broken.RVC3/tools/pvplus.py, not baretools/orfigures/3d/: it's imported by ~40 scripts acrossfigures/3d/and severalfigures/code/chapterN/subdirectories, andRVC3.toolsis already a real, pip-installable package — the only placement that resolves for every consumer via a normalpip install -e .dev setup, noPYTHONPATHneeded.import pvplus→from RVC3.tools import pvplusin all 40 consuming scripts (mechanical, one line each).outfile()'s hardcoded personal absolute path to derive from the calling script's own location — same class of bug as the original issue.PolyData.transform()now requiring an explicitinplace=kwarg (used to default toTrue) at all 7 call sites inpvplus.py, discovered while actually testing this against a current pyvista install.rvcprintimport from the 6figures/3d/*.pyscripts that had it (figures/3d/README.mdconfirms these scripts save output viaplotter.export_gltf(...)directly, never viarvcprint). Left thefigures/code/chapterN/copies of these scripts untouched —figures/code/README.mddocumentsrvcprintas the required mechanism those scripts use to save the book's actual PDF figures, so it was never broken there and out of scope for this fix.figuresoptional-dependency extra (pyvista,vtk,pillow) topyproject.toml— previously undeclared anywhere.pip install -e ".[figures]"is now the way to get set up for regenerating book figures.Left alone, out of scope here: several individual figure scripts (mostly the
fig7_*arm-kinematics figures, andfig2_24.py) have their own local.transform()calls with the same pyvista API-drift issue.Test plan
py_compileadd_frame,ribbon,axis,add_ellipsoid,ellipsoid_3d,add_frame2all exercised), all exit 0off_screen=True) — confirmed rendering correctly by eye, not run to completion headlesslyfigures/code/chapterN/copies confirmed byte-identical toorigin/mainapart from the one-linepvplusimport fix