Conversation
Code ReviewI've reviewed this PR and found 3 issues that need to be addressed: 1. Missing Type Hints (CLAUDE.md violation)Location: All functions in The functions lack type hints, which violates CLAUDE.md Section 4.1:
For example, def setup_plot_style() -> None:
"""Set up matplotlib style for all plots."""
plt.style.use('seaborn-v0_8-darkgrid')
plt.rcParams['figure.figsize'] = (12, 8)This applies to all 10 functions in the file. 2. Docstrings Not in Google-Style Format (CLAUDE.md violation)Location: All functions in The docstrings don't follow Google-style format as required by CLAUDE.md Section 4.1:
Current format (example from def load_data(output_dir):
"""Load observation and posterior data from the output directory.
Args:
output_dir: Path to directory containing observation.npz and posterior.nc
Returns:
tuple: (obs_data, idata, posterior)
"""Should be: def load_data(output_dir: Path) -> tuple[dict, az.InferenceData, xr.Dataset]:
"""Load observation and posterior data from the output directory.
Args:
output_dir (Path): Path to directory containing observation.npz and posterior.nc
Returns:
tuple: A tuple containing:
- obs_data (dict): Loaded observation data from observation.npz
- idata (az.InferenceData): ArviZ InferenceData object
- posterior (xr.Dataset): Posterior dataset
Raises:
FileNotFoundError: If observation.npz or posterior.nc not found
"""Google-style requires type annotations in the Args section and more detailed Returns sections. 3. Security Issue - Automatic Package InstallationLocation: Lines 202-205 and 266-269 in The code automatically installs the SHINE/shine/plot_shine_results.py Lines 199 to 207 in a2ce2d4 This is problematic because:
Recommended fix: Either:
This same pattern appears twice in the file (in both Summary: Please address these 3 issues before merging. The first two are code standards compliance issues from CLAUDE.md, and the third is a security/best-practices concern. |
|
@claude can you address the issues found during the review? |
|
Claude finished @EiffL's task in 2m 16s —— View job Code Review Issues Fixed ✅All 3 issues identified in the code review have been addressed and pushed to the Changes Made1. Added Type Hints (PEP 484 Compliance)
2. Converted Docstrings to Google-Style Format
3. Removed Automatic Package Installation (Security Issue)
Commit DetailsCommit: The changes ensure full compliance with CLAUDE.md requirements and follow security best practices. |
- Add full PEP 484 type hints to all functions - Convert all docstrings to Google-style format with detailed Args, Returns, and Raises sections - Remove automatic package installation security issue: replaced with ImportError messages - User must now manually install 'corner' package if needed Addresses review feedback on PR #15. Co-authored-by: Francois Lanusse <EiffL@users.noreply.github.com>
|
@claude corner is a great package, very useful, can you add it as a dependency of the package? we will certainly use it more that once |
|
Claude finished @EiffL's task in 58s —— View job Adding corner as a Package Dependency ✅Successfully added Tasks
Changes MadeAdded dependencies = [
"jax>=0.4.0",
"numpyro>=0.13.0",
"galsim>=2.5.0",
"jax-galsim>=2026.2.0",
"pydantic>=2.0.0",
"pyyaml>=6.0",
"arviz>=0.17.0",
"matplotlib>=3.7.0",
"corner>=2.0.0", # Added for corner plots and visualization
]The Commit Details
--- | Branch: dev-plot-results |
Added corner>=2.0.0 to project dependencies in pyproject.toml for visualization and plotting functionality. Co-authored-by: Francois Lanusse <EiffL@users.noreply.github.com>
|
@claude hum, this is a plotting script, it shouldn't be in the package itself. Can you add a scripts folder where such utility scripts can be stored? |
|
Claude finished @EiffL's task in 2m 43s —— View job Moving Plotting Script to Scripts Folder ✅Successfully moved Tasks
Changes MadeThe plotting script has been relocated from the package to a utility scripts directory: Old location: Git has preserved the file's history by detecting this as a rename operation (98% similarity). UsageThe script can now be run using: python scripts/plot_shine_results.py --output my_output/Commit Details
|
Utility scripts should be kept separate from the package itself. Created scripts/ directory for utility scripts. Co-authored-by: Francois Lanusse <EiffL@users.noreply.github.com>
Add a python script that can be used as follow:
python shine/plot_shine_results.py --output my_output/Features:
✅ Reads observation.npz and posterior.nc from the specified directory
✅ Saves all plots to the same directory
✅ Generates visualizations:
✅ Clean terminal output with progress indicators
✅ Proper error handling if files are missing
✅ Auto-installs the corner package if needed