fix(cli): fix jascome #67
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR fixes the JASCOME command in the CLI by adding visualization capabilities and a new accuracy benchmarking function. The changes introduce coordinate system visualization for different branching types and reorganize output into a dedicated directory.
Key Changes:
- Added
ultrasphere[plot]dependency withpydotandpygraphvizfor graph visualization - Created visualization code for spherical coordinate branching types (generates SVG files)
- Organized JASCOME output into a dedicated
jascome/directory - Added new
accuracyfunction for numerical accuracy benchmarking across different configurations
Reviewed changes
Copilot reviewed 5 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pyproject.toml |
Added ultrasphere[plot]>=2.0.4 dependency for plotting capabilities |
uv.lock |
Updated lock file with new dependencies: pydot 4.0.1 and pygraphviz 1.14 |
src/biem_helmholtz_sphere/cli.py |
Added matplotlib/networkx imports, directory organization, coordinate visualization, and new accuracy benchmarking function |
jascome/*.csv |
Updated numerical output data reflecting computational changes from the fixes |
*.svg files |
Generated coordinate system visualizations for different branching types (caa, bpbpa, bpa, bba, ba) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/biem_helmholtz_sphere/cli.py
Outdated
| branching_types: str = "a", | ||
| ) -> None: | ||
| """Numerical examples for JASCOME.""" | ||
| branchin_types = branching_types.split(",") |
There was a problem hiding this comment.
Typo in variable name: branchin_types should be branching_types (missing 'g'). This inconsistency with the parameter name could cause confusion.
src/biem_helmholtz_sphere/cli.py
Outdated
| uscat = calc.uscat(xp.asarray((0.0,) * c.c_ndim, device=device, dtype=dtype)) | ||
| with Path("accuracy/accuracy.csv").open("a") as f: | ||
| f.write( | ||
| f"{btype},{n_end},{complex(uscat)},{k}," |
There was a problem hiding this comment.
The order of arguments in the CSV write is incorrect. The format string specifies branching_types,n_end,k,uscat,... but the write statement outputs {btype},{n_end},{complex(uscat)},{k},... - the k value appears after uscat instead of before it. This should be: f"{btype},{n_end},{k},{complex(uscat)},..." to match the header.
| f"{btype},{n_end},{complex(uscat)},{k}," | |
| f"{btype},{n_end},{k},{complex(uscat)}," |
| fig, ax = plt.subplots() | ||
| draw(c, ax=ax) | ||
| fig.savefig(f"{btype}.svg") |
There was a problem hiding this comment.
The figure is saved using the branching type as filename without sanitization. Consider using Path(f"jascome/{btype}.svg") or validating btype to ensure it's safe for filesystem use, especially if it could contain special characters like path separators.
Description of change
Pull-Request Checklist
mainbranchFixes #0000