style: apply ruff and black formatting fixes - #19
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (3)
You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR standardizes string/formatting across the codebase, reorganizes and expands the public API surfaces (core and pypath), refactors Mixed Trophic Impacts and Ecosim summary fields, extends scenario/forcing dataclasses, and adds a new Shiny example app and multiple UI/layout adjustments. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
CI Auto-Fix ran for workflow run 20698723858: formatting fixes applied or diagnostic PR created. |
|
CI Auto-Fix ran for workflow run 20698732792: formatting fixes applied or diagnostic PR created. |
|
CI Auto-Fix ran for workflow run 20699379807: formatting fixes applied or diagnostic PR created. |
|
CI Auto-Fix ran for workflow run 20699391493: formatting fixes applied or diagnostic PR created. |
|
CI Auto-Fix ran for workflow run 20699462038: formatting fixes applied or diagnostic PR created. |
…REY_SWITCHING_POWER
…ts) and minor type checks
|
CI Auto-Fix ran for workflow run 20699710292: formatting fixes applied or diagnostic PR created. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
benchmark_spatial_optimizations.py (1)
97-97: Fix unused variable flagged by pipeline.The
fluxvariable is assigned but never used, causing the CI pipeline failure (F841). Since this is a benchmark measuring execution time rather than the result, prefix with underscore to indicate it's intentionally unused.🔎 Proposed fix
- flux = diffusion_flux(biomass, dispersal_rate, grid, grid.adjacency_matrix) + _flux = diffusion_flux(biomass, dispersal_rate, grid, grid.adjacency_matrix)app/pages/diet_rewiring_demo.py (2)
244-257: Documentation references unavailable function.The code examples reference
create_diet_rewiring(), but onlyDietRewiringis imported (Line 19) and the actual implementation usesDietRewiring()constructor directly (Line 385). Users following this example will encounter import errors.🔎 Suggested fix: Update examples to match actual API
### Fish Predators ```python # Cod switching between herring and sprat - diet_rewiring = create_diet_rewiring( + diet_rewiring = DietRewiring( + enabled=True, switching_power=2.5, # Moderate opportunism update_interval=3 # Quarterly )Apply similar changes to the other examples at lines 252-257 and 262-266.
638-657: Generated code example uses incorrect import.The generated code example references
create_diet_rewiring(Line 641), but based on the actual implementation (Line 385) and imports (Line 19), the correct usage isDietRewiring()constructor.🔎 Suggested fix: Update generated code template
code = f"""# Dynamic Diet Rewiring Example # Generated from PyPath Demo -from pypath.core.forcing import create_diet_rewiring +from pypath.core.forcing import DietRewiring from pypath.core.ecosim_advanced import rsim_run_advanced # Create diet rewiring configuration -diet_rewiring = create_diet_rewiring( +diet_rewiring = DietRewiring( + enabled=True, switching_power={switching_power}, min_proportion={min_proportion}, update_interval={update_interval} )src/pypath/core/adjustments.py (1)
406-410: Remove unused variable flagged by CI.The variable
n_yearsis assigned but never used, as reported by the pipeline failure (F841). This should be removed to clean up the code.🔎 Suggested fix
if parameter in ["ForcedEffort"]: # Monthly matrix - get base years - n_years = scenario.fishing.ForcedEffort.shape[0] // 12 start_year = 1 # Assume 1-based years return [y - start_year for y in years]app/pages/analysis.py (1)
493-503: Pass group names, not themodelobject, toplot_mti_heatmap
plot_mti_heatmapexpects a sequence of group-name labels, butmti_heatmap_plotpasses the entiremodelobject. SinceRpathis not iterable/sequence-like, this is likely to raiseTypeErroror produce incorrect axis labels.Proposed fix for `mti_heatmap_plot`
- try: - fig = plot_mti_heatmap(mti, model) + try: + # Use group names from the underlying params; trim to MTI size + group_names = model.params.model["Group"].values + n = mti.shape[0] + group_names = list(group_names[:n]) + fig = plot_mti_heatmap(mti, group_names)src/pypath/core/analysis.py (1)
404-448: Align Ecosim analysis helpers withRsimOutputfield names (annual_Biomass/annual_Catch)Several functions (
summarize_ecosim_output,compare_scenarios,check_ecosim_stability,export_ecosim_to_dataframe) readoutput.out_Biomass_annualandoutput.out_Catch_annual, butRsimOutputexposesannual_Biomassandannual_Catch. These mismatches will raiseAttributeErrorwhen called.Proposed field-name corrections
def summarize_ecosim_output( output: RsimOutput, scenario: Optional[RsimScenario] = None ) -> EcosimSummary: @@ - biomass = output.out_Biomass_annual - catch = output.out_Catch_annual + biomass = output.annual_Biomass + catch = output.annual_Catch @@ def compare_scenarios( @@ - n_groups = outputs[0].out_Biomass_annual.shape[1] + n_groups = outputs[0].annual_Biomass.shape[1] @@ - start = output.out_Biomass_annual[0, groups] - end = output.out_Biomass_annual[-1, groups] + start = output.annual_Biomass[0, groups] + end = output.annual_Biomass[-1, groups] @@ - biomass = output.out_Biomass_annual + biomass = output.annual_Biomass @@ def export_ecosim_to_dataframe( @@ - n_years, n_groups = output.out_Biomass_annual.shape + n_years, n_groups = output.annual_Biomass.shape @@ - biomass_df = pd.DataFrame( - output.out_Biomass_annual[:, 1:], columns=names, index=range(1, n_years + 1) - ) + biomass_df = pd.DataFrame( + output.annual_Biomass[:, 1:], columns=names, index=range(1, n_years + 1) + ) @@ - catch_df = pd.DataFrame( - output.out_Catch_annual[:, 1:], columns=names, index=range(1, n_years + 1) - ) + catch_df = pd.DataFrame( + output.annual_Catch[:, 1:], columns=names, index=range(1, n_years + 1) + )The existing monthly export already uses
output.out_Biomassand can remain unchanged.Also applies to: 473-488, 607-613, 717-747
src/pypath/__init__.py (1)
11-158: PR description is misleading: import reorganization is not purely formatting.The PR claims "formatting-only changes," but the diff shows:
- Import blocks reordered: params → adjustments (main order) vs. adjustments → params (current order)
- Within-block sorting: imports alphabetized within each block (e.g., ecosim imports sorted: RsimFishing, RsimForcing, RsimOutput, RsimParams instead of RsimParams, RsimState, RsimForcing)
The alphabetization aligns with Black/ruff formatter expectations, but reorganizing import block order is a structural change beyond formatting. The
__all__list remains identical, so there are no API-breaking changes—but the PR should be labeled as an "import reorganization + formatting" change, not "formatting-only."Update the PR description or provide justification for the import block reordering.
🧹 Nitpick comments (7)
scripts/test_database_connections.py (1)
23-39: LGTM: Improved error reporting.The addition of
IMPORT_ERRORto capture and display import failure details is a helpful improvement for debugging.Optional: Initialize IMPORT_ERROR before try block for defensive programming
While the current implementation is safe (IMPORT_ERROR is only accessed when the except block has executed), initializing it before the try block would make the code more robust against future refactoring:
# Add src to path sys.path.insert(0, str(Path(__file__).parent.parent / "src")) +IMPORT_ERROR = None try: from pypath.io.biodata import (app/pages/optimization_demo.py (2)
484-484: Consider removing unused variable.
_best_yis computed but never referenced. If this was intended to document the logic or silence a warning, consider removing it for clarity.🔎 Suggested cleanup
- _best_y = np.min(y) best_idx = np.argmin(y)
585-585: Consider removing unused variable.
_x_plotis created but never used in the plotting logic. This appears to be leftover from a more complex GP visualization.🔎 Suggested cleanup
- # Create dense grid for plotting - _x_plot = np.linspace(1.0, 3.0, 200) - # Simplified GP visualization (real would use actual GP predictions)example_shiny.py (1)
8-19: Consider adding input validation.The sidebar accepts user inputs but doesn't appear to validate them. Consider adding validation for:
- Region selection (ensure valid options)
- Data points slider (already constrained by min/max)
app/pages/diet_rewiring_demo.py (1)
354-354: Unused reactive value:_diet_historyis never referenced.The reactive value
_diet_historyis initialized but never used anywhere in this file. This appears to be leftover from refactoring.🔎 Suggested cleanup
- _diet_history = reactive.Value(None)app/pages/data_import.py (1)
979-991: Guard against missing biodiversity result columns inbiodata_fetch_status
biodata_fetch_statusassumes"trophic_level"and"occurrence_count"always exist; ifbatch_get_species_infoever omits one, this will raise aKeyError. A small defensive check would make this more robust.Proposed defensive update
- n_species = len(df) - n_with_tl = df["trophic_level"].notna().sum() - n_with_obis = df["occurrence_count"].notna().sum() + n_species = len(df) + n_with_tl = ( + df["trophic_level"].notna().sum() + if "trophic_level" in df.columns + else 0 + ) + n_with_obis = ( + df["occurrence_count"].notna().sum() + if "occurrence_count" in df.columns + else 0 + )app/pages/analysis.py (1)
588-603: Exclude keystoneness index dummy element and align with group names
keystoneness_indexreturns an array with index 0 unused (len = n_groups + 1), butkeystoneness_tableuses the whole array againstgroups[: len(ks)]. This mixes the dummy element with real groups and can misalign names.Proposed keystoneness table alignment
- try: - groups = model.params.model["Group"].values - - # Create DataFrame with group names - df = pd.DataFrame( - {"Group": groups[: len(ks)], "Keystoneness": np.round(ks, 4)} - ) + try: + groups = model.params.model["Group"].values + # Skip index 0 (unused) and align lengths safely + n = min(len(groups), len(ks) - 1) + ks_vals = ks[1 : n + 1] + + df = pd.DataFrame( + {"Group": groups[:n], "Keystoneness": np.round(ks_vals, 4)} + )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (111)
.github/workflows/ci-auto-fix.ymlapp/__init__.pyapp/app.pyapp/config.pyapp/logger.pyapp/pages/__init__.pyapp/pages/about.pyapp/pages/analysis.pyapp/pages/data_import.pyapp/pages/diet_rewiring_demo.pyapp/pages/ecopath.pyapp/pages/ecosim.pyapp/pages/ecospace.pyapp/pages/forcing_demo.pyapp/pages/home.pyapp/pages/multistanza.pyapp/pages/optimization_demo.pyapp/pages/prebalance.pyapp/pages/results.pyapp/pages/utils.pyapp/pages/validation.pybenchmark_spatial_optimizations.pycreate_example_model.pydemo_advanced_features.pyexample_shiny.pyexamples/ecospace_demo.pygenerate_test_timeseries.pypages/__init__.pyrun_app.pyscripts/run_extract_rpath.pyscripts/test_database_connections.pysrc/pypath/__init__.pysrc/pypath/analysis/__init__.pysrc/pypath/analysis/prebalance.pysrc/pypath/core/__init__.pysrc/pypath/core/adjustments.pysrc/pypath/core/analysis.pysrc/pypath/core/autofix.pysrc/pypath/core/constants.pysrc/pypath/core/ecopath.pysrc/pypath/core/ecosim.pysrc/pypath/core/ecosim_advanced.pysrc/pypath/core/ecosim_deriv.pysrc/pypath/core/forcing.pysrc/pypath/core/optimization.pysrc/pypath/core/params.pysrc/pypath/core/plotting.pysrc/pypath/core/stanzas.pysrc/pypath/io/__init__.pysrc/pypath/io/biodata.pysrc/pypath/io/ecobase.pysrc/pypath/io/ewemdb.pysrc/pypath/io/utils.pysrc/pypath/spatial/__init__.pysrc/pypath/spatial/connectivity.pysrc/pypath/spatial/dispersal.pysrc/pypath/spatial/ecospace_params.pysrc/pypath/spatial/environmental.pysrc/pypath/spatial/external_flux.pysrc/pypath/spatial/fishing.pysrc/pypath/spatial/gis_utils.pysrc/pypath/spatial/habitat.pysrc/pypath/spatial/integration.pytest_advanced_features.pytest_biodata_workflow.pytest_data_sync.pytest_pb_simple.pytest_pb_validation_fix.pytests/test_adjustments.pytests/test_analysis.pytests/test_app_import.pytests/test_backward_compatibility.pytests/test_biodata.pytests/test_biodata_integration.pytests/test_diet_rewiring.pytests/test_dispersal.pytests/test_ecobase.pytests/test_ecopath.pytests/test_ecopath_input_conversion.pytests/test_ecosim.pytests/test_ecosim_model_type.pytests/test_ecosim_qlink.pytests/test_ecosim_stanzas.pytests/test_environmental.pytests/test_ewemdb.pytests/test_file_format_support.pytests/test_forcing.pytests/test_grid_creation.pytests/test_habitat.pytests/test_hexagonal_grids.pytests/test_import_diet.pytests/test_irregular_grids.pytests/test_lt_model.pytests/test_optimization_integration.pytests/test_optimization_scenarios.pytests/test_optimization_unit.pytests/test_plotting.pytests/test_rpath_compatibility.pytests/test_rpath_ecosim_core.pytests/test_rpath_reference.pytests/test_shiny_app.pytests/test_shiny_pages.pytests/test_shiny_reactive.pytests/test_spatial_ecosim_integration.pytests/test_spatial_fishing.pytests/test_spatial_integration.pytests/test_spatial_performance.pytests/test_spatial_validation.pytests/test_stanzas.pyverify_biodata_deps.pyverify_ecospace.py
🧰 Additional context used
🧬 Code graph analysis (18)
src/pypath/core/autofix.py (2)
src/pypath/core/ecopath.py (2)
Rpath(54-168)rpath(171-616)src/pypath/core/ecosim.py (2)
RsimParams(38-173)RsimScenario(254-291)
app/pages/ecosim.py (5)
src/pypath/core/autofix.py (1)
validate_and_fix_scenario(283-365)src/pypath/core/ecosim.py (3)
RsimScenario(254-291)rsim_run(837-1126)rsim_scenario(771-834)src/pypath/core/params.py (1)
create_rpath_params(102-246)src/pypath/core/forcing.py (1)
DietRewiring(253-373)src/pypath/core/ecosim_advanced.py (1)
rsim_run_advanced(123-362)
benchmark_spatial_optimizations.py (2)
src/pypath/spatial/dispersal.py (1)
diffusion_flux(26-121)src/pypath/spatial/environmental.py (1)
n_patches(208-212)
app/pages/forcing_demo.py (1)
src/pypath/core/forcing.py (2)
StateForcing(103-249)create_recruitment_forcing(424-465)
examples/ecospace_demo.py (3)
src/pypath/spatial/fishing.py (3)
allocate_gravity(127-204)allocate_port_based(207-284)allocate_uniform(104-124)src/pypath/spatial/gis_utils.py (1)
create_1d_grid(206-257)src/pypath/spatial/environmental.py (1)
n_patches(208-212)
src/pypath/core/ecosim_advanced.py (2)
src/pypath/core/ecosim.py (2)
RsimOutput(295-349)RsimScenario(254-291)src/pypath/core/forcing.py (4)
DietRewiring(253-373)ForcingMode(21-27)StateForcing(103-249)StateVariable(30-39)
src/pypath/core/ecosim.py (1)
src/pypath/core/stanzas.py (3)
RsimStanzas(91-126)rsim_stanzas(353-529)split_set_pred(619-657)
app/pages/results.py (2)
app/pages/utils.py (1)
get_model_info(495-609)src/pypath/core/ecopath.py (1)
summary(146-168)
app/pages/home.py (2)
src/pypath/core/params.py (1)
create_rpath_params(102-246)tests/test_rpath_compatibility.py (1)
make_diet(345-351)
demo_advanced_features.py (1)
src/pypath/core/forcing.py (6)
StateForcing(103-249)create_biomass_forcing(376-421)create_recruitment_forcing(424-465)get_value(72-99)initialize(282-291)add_forcing(114-194)
app/pages/analysis.py (2)
src/pypath/core/analysis.py (4)
check_ecopath_balance(496-562)export_ecopath_to_dataframe(640-694)keystoneness_index(108-150)mixed_trophic_impacts(30-105)src/pypath/core/plotting.py (3)
plot_foodweb(49-219)plot_mti_heatmap(517-577)plot_trophic_spectrum(436-514)
app/pages/diet_rewiring_demo.py (1)
src/pypath/core/forcing.py (1)
DietRewiring(253-373)
app/pages/ecospace.py (5)
src/pypath/spatial/ecospace_params.py (1)
EcospaceGrid(30-187)src/pypath/spatial/fishing.py (3)
allocate_gravity(127-204)allocate_port_based(207-284)allocate_uniform(104-124)src/pypath/spatial/gis_utils.py (2)
create_1d_grid(206-257)create_regular_grid(123-203)src/pypath/spatial/connectivity.py (1)
build_adjacency_from_gdf(28-114)src/pypath/spatial/environmental.py (1)
n_patches(208-212)
src/pypath/analysis/__init__.py (1)
src/pypath/analysis/prebalance.py (5)
calculate_biomass_slope(85-126)calculate_predator_prey_ratios(153-210)calculate_vital_rate_ratios(213-283)generate_prebalance_report(416-478)print_prebalance_summary(481-520)
app/pages/ecopath.py (4)
src/pypath/core/ecopath.py (3)
Rpath(54-168)rpath(171-616)summary(146-168)src/pypath/core/params.py (2)
RpathParams(39-99)create_rpath_params(102-246)app/pages/utils.py (3)
create_cell_styles(301-487)format_dataframe_for_display(174-298)is_balanced_model(83-107)app/pages/validation.py (4)
validate_biomass(65-119)validate_ee(181-225)validate_model_parameters(228-311)validate_pb(122-178)
generate_test_timeseries.py (2)
src/pypath/core/ecosim.py (2)
rsim_run(837-1126)rsim_scenario(771-834)src/pypath/io/ewemdb.py (1)
read_ewemdb(315-931)
src/pypath/core/__init__.py (3)
src/pypath/core/adjustments.py (1)
adjust_scenario(210-239)src/pypath/core/autofix.py (3)
AutofixResult(32-50)diagnose_crash_causes(53-183)validate_and_fix_scenario(283-365)src/pypath/core/params.py (2)
RpathParams(39-99)check_rpath_params(338-422)
src/pypath/core/analysis.py (2)
src/pypath/core/ecopath.py (2)
Rpath(54-168)rpath(171-616)src/pypath/core/ecosim.py (2)
RsimOutput(295-349)RsimScenario(254-291)
🪛 GitHub Actions: CI
benchmark_spatial_optimizations.py
[error] 97-97: F841 Local variable flux is assigned to but never used
app/pages/results.py
[error] 9-9: F401 app.config.COLORS imported but unused
demo_advanced_features.py
[error] 103-103: F841 Local variable forcing is assigned to but never used
app/pages/prebalance.py
[error] 27-27: E402 Module level import not at top of file
[error] 33-33: E402 Module level import not at top of file
src/pypath/core/adjustments.py
[error] 408-408: F841 Local variable n_years is assigned to but never used
src/pypath/core/analysis.py
[error] 81-81: E741 Ambiguous variable name: I
[error] 88-88: F841 Local variable inv_IDC is assigned to but never used
[error] 101-101: F841 Local variable inv_IQ is assigned to but never used
[error] 550-550: F841 Local variable consumption is assigned to but never used
[error] 551-551: F841 Local variable production is assigned to but never used
🔇 Additional comments (58)
scripts/test_database_connections.py (1)
45-50: Formatting changes look good.The string quote normalization and formatting adjustments are consistent throughout the file and align with ruff and black standards. No logic changes introduced by the formatting.
Also applies to: 98-103, 111-117, 162-168, 220-227, 290-297, 351-357, 428-436, 463-469, 475-483
app/pages/optimization_demo.py (6)
8-8: LGTM - pandas import is used throughout the file.The pandas import is properly used for DataFrame creation in multiple locations (lines 407, 425, 692, 698).
19-377: LGTM - UI formatting changes are consistent.All changes in the UI definition are purely stylistic (quote normalization, trailing commas, indentation) with no behavioral impact.
407-407: LGTM - DataFrame creation formatting.Quote normalization applied consistently to dictionary literals with no logic changes.
Also applies to: 425-425
446-473: LGTM - objective function formatting improves readability.Multi-line formatting of complex expressions makes the code more maintainable while preserving all mathematical logic.
499-505: LGTM - plotting and results code formatting is consistent.All dictionary literals, Plotly configurations, and DataFrame constructions have been formatted consistently with no logic changes. The multi-line formatting improves code readability.
Also applies to: 527-536, 595-614, 653-671, 698-710
716-773: LGTM - code generation template unchanged.The formatting changes to the code generation function don't affect the generated output.
app/config.py (1)
17-17: LGTM! Formatting changes are consistent.The string literal conversions from single to double quotes throughout the file are consistent with the Black formatting standards and align with the PR's objective to apply formatting fixes.
Also applies to: 25-25, 35-35, 44-46, 55-76, 85-98, 120-120
app/pages/__init__.py (1)
10-22: LGTM! Formatting changes are consistent.The conversion to double quotes and trailing comma addition align with Black formatting standards.
Also applies to: 48-48
src/pypath/core/constants.py (1)
179-179: LGTM! Formatting change is consistent.The string literal conversion to double quotes aligns with Black formatting standards.
pages/__init__.py (1)
9-9: LGTM! Formatting changes are consistent.The string literal conversions to double quotes align with Black formatting standards.
Also applies to: 14-14, 17-17, 20-20, 22-22
demo_advanced_features.py (1)
13-16: LGTM! Most formatting changes are consistent.The string literal conversions to double quotes and import reorganization align with Black and ruff formatting standards. The majority of changes in this file are formatting-only and consistent with the PR objectives.
However, note the pipeline failure at line 103 (see separate comment).
Also applies to: 21-26, 31-370
benchmark_spatial_optimizations.py (1)
10-19: LGTM!Import reorganization and formatting changes align with the PR's style normalization objectives.
app/pages/about.py (1)
6-213: LGTM!Formatting and content updates are appropriate. The addition of
target="_blank"for external links improves user experience by opening them in new tabs..github/workflows/ci-auto-fix.yml (1)
29-34: LGTM!The change to Python module invocations (
python -m ruff,python -m black) is more reliable in CI environments where the tool binaries might not be in PATH. The separation ofruff check --fix(linting autofixes) andruff format(code formatting) is the correct approach.app/pages/utils.py (1)
1-607: LGTM!Extensive formatting changes (quote normalization, multi-line formatting, whitespace adjustments) without any logic modifications. The changes improve consistency with the project-wide style conventions.
src/pypath/analysis/__init__.py (1)
6-26: LGTM!Import reordering and quote normalization in
__all__are purely stylistic changes with no impact on functionality.scripts/run_extract_rpath.py (1)
1-105: LGTM!Formatting changes (quote normalization, whitespace adjustments) are consistent with the PR's style objectives. No functional changes.
app/pages/validation.py (1)
1-310: LGTM!Formatting changes throughout the file (multi-line function signatures, quote normalization, whitespace adjustments) improve readability while preserving all validation logic.
app/pages/ecospace.py (1)
12-46: LGTM - Formatting changes are consistent and appropriate.The import reorganization and string literal standardization (single quotes → double quotes) throughout this file align with the PR's formatting objectives. The changes maintain all functional behavior while improving code consistency.
app/pages/ecosim.py (1)
3-217: LGTM - Formatting improvements are well-executed.The formatting updates throughout this file (import organization, quote standardization, spacing) are consistent with the PR's objectives. The enhanced docstring for
ecosim_server(lines 412-434) is a nice addition that improves code documentation.src/pypath/core/autofix.py (2)
31-50: Note: Public API change mixed with formatting.The addition of the
original_params: dictfield to theAutofixResultdataclass is a public API change rather than pure formatting. This new required field is properly utilized in the code (line 277), but it's worth noting that this change could affect external code that instantiatesAutofixResultdirectly.For future PRs, consider separating API changes from formatting updates to maintain clearer change history.
9-26: LGTM - Import organization and type hints improved.The reformatted imports and added type hints enhance code clarity and align with modern Python best practices.
generate_test_timeseries.py (1)
24-32: LGTM - Type annotations enhance code clarity.The addition of complete type annotations to the function signature improves code documentation and enables better IDE support and static analysis. The formatting changes throughout the file are consistent with the PR's objectives.
src/pypath/analysis/prebalance.py (1)
10-17: LGTM - Import organization and type hints are well-structured.The reorganized imports with added type hints from the
typingmodule improve code clarity and enable better static analysis. All formatting changes throughout the file align with the PR's standardization objectives.app/pages/multistanza.py (1)
1-444: LGTM! Formatting changes only.All changes in this file are consistent with ruff and black formatting:
- String quote normalization (single → double quotes)
- Trailing comma additions
- Multi-line formatting improvements
- Import reorganization
No functional logic changes detected.
app/pages/prebalance.py (1)
1-561: Approve formatting improvements.The string quote normalization (single → double quotes), multi-line formatting adjustments, and other stylistic changes throughout the file are consistent with ruff and black formatting standards.
app/pages/forcing_demo.py (1)
1-619: LGTM with minor note.The formatting changes are consistent with ruff and black standards. The addition of
StateForcingto the imports (line 15) appears necessary since it's used in the code at line 367. If this import was previously missing, this would be a bug fix rather than a formatting-only change.examples/ecospace_demo.py (2)
1-390: Approve formatting improvements.Apart from the API changes noted above, the string quote normalization, multi-line formatting, and other stylistic adjustments are consistent with ruff and black standards.
280-309: The new allocation API functions are properly implemented and tested.All three functions (
allocate_uniform,allocate_gravity,allocate_port_based) are fully implemented insrc/pypath/spatial/fishing.pywith comprehensive docstrings, type hints, and examples. They are correctly exported in the module's__init__.pyand properly imported in the demo file. The comprehensive test suite intests/test_spatial_fishing.pycovers all major functionality including edge cases (zero biomass fallback, max distance cutoffs, multiple ports) and parameter variations (alpha/beta exponents). Function signatures match all usage patterns in the code snippet.example_shiny.py (1)
67-71: Fixed random seed may not be intended for production.Line 69 uses
np.random.seed(42)which fixes the random data generation. This is appropriate for a demo/example, but ensure users understand this should be removed or made configurable for real applications.app/__init__.py (1)
1-19: LGTM!The blank line addition improves readability with no functional impact.
run_app.py (1)
41-43: Good addition of development mode warning.The warning message for
--reloadmode clearly communicates that this is for development only, which helps prevent misuse in production environments.app/logger.py (1)
39-54: Well-implemented logger utility function.The new
get_logger()function provides a clean interface for obtaining namespaced child loggers, following Python logging best practices. The docstring is clear and the implementation is straightforward.src/pypath/core/ecopath.py (7)
10-10: LGTM - Import addition is formatting-related.The
dataclassesimport on line 10 is consistent with the@dataclassdecorator used on theRpathclass at line 53.
18-50: LGTM - Custom solver implementation looks correct.The
_gauss_solvefunction provides a pure-Python fallback for small linear systems. The implementation correctly handles partial pivoting and checks for singular matrices.
129-144: Verify slice notation change.Line 129 changes the slice from
self.EE[:self.NUM_LIVING + self.NUM_DEAD]which is correct. The formatting adjustment maintains the same logic.
156-168: LGTM - Dictionary formatting is consistent.The change from single-quoted to double-quoted keys (e.g.,
"Group","Type") is purely stylistic and maintains correct dictionary structure.
221-244: LGTM - Column access formatting is consistent.Changes from
model_df['Type']tomodel_df["Type"]are purely stylistic. The logic for extracting group types and creating index arrays remains unchanged.
358-376: LGTM - Multi-line formatting improves readability.The reformatting of the diagonal element calculation (lines 372-376) across multiple lines improves readability without changing the conditional logic.
581-583: LGTM - Error state context unchanged.The
np.errstatecontext manager formatting change (using double quotes) is stylistic only and maintains the same behavior for handling divide-by-zero warnings.src/pypath/core/ecosim_advanced.py (4)
83-86: LGTM - Unused variable correctly prefixed.The variable
_scaleis calculated but not used in theRESCALEmode logic. Prefixing with underscore correctly signals this is intentional (silencing unused-variable warning as noted in commit messages).
182-182: LGTM - Unused variable correctly prefixed.The
_fishingvariable is extracted fromscenario.fishingbut never used in this simplified demonstration implementation. The underscore prefix correctly signals this is intentional.
296-330: LGTM - End state construction is thorough.The expanded end-state construction (lines 296-330) properly handles optional fields (
SpawnBio,StanzaPred,EggsStanza, etc.) by copying from start_state when available or setting to None. This is more defensive than the previous implementation.
400-405: LGTM - Export list formatting is clear.The
__all__list reformatting with one item per line and double quotes improves readability and is consistent with Python conventions.app/pages/ecopath.py (4)
72-79: LGTM - Attribute access is correct.The changes from
hasattr(model, 'Group')tohasattr(model, "Group")and frommodel['Group']tomodel["Group"]are purely stylistic. The logic correctly handles bothRpathobjects (withGroupattribute) andRpathParamsobjects (withmodelDataFrame).
137-156: LGTM - Type extraction logic preserved.Lines 137-156 correctly extract types from either balanced
Rpathmodels (withtypeattribute) or unbalancedRpathParams(withTypecolumn in model DataFrame). The double-quote formatting doesn't affect the logic.
456-507: LGTM - Rendering logic is well-structured.The model parameters table rendering properly handles:
- Missing parameters (lines 459-462)
- Column selection (lines 465-476)
- Remarks and stanza groups (lines 480-500)
- Cell styling (lines 501-507)
The formatting changes improve readability without altering behavior.
789-837: LGTM - Edit handling includes proper validation.The cell edit handler correctly:
- Extracts row, column, and value from the edit event
- Validates based on parameter type (Biomass, PB, EE)
- Shows appropriate notifications
- Handles numeric conversion errors
The double-quote formatting is consistent throughout.
app/app.py (3)
8-17: LGTM - Logging setup is correct.The addition of
loggingandsysimports (lines 8-9) and logger creation (line 17) properly sets up logging infrastructure. The logger is used in the error handler at line 313.
36-69: LGTM - Import organization is complete.The expanded import list includes all required page modules:
- Core pages: home, data_import, ecopath, prebalance, ecosim, ecospace
- Advanced features: multistanza, forcing_demo, diet_rewiring_demo, optimization_demo
- Analysis pages: analysis, results, about
Both the
tryblock (app.pages import) andexceptfallback (pages import) have identical module lists, ensuring import resolution works in different execution contexts.
309-313: Good addition - Error handling improves robustness.The try-except wrapper around server initialization (lines 309-313) is a valuable improvement that:
- Catches initialization errors per page
- Logs detailed error information with traceback
- Prevents one failing page from breaking the entire app
This is a functional improvement beyond just formatting.
app/pages/home.py (4)
5-21: LGTM - Import organization is clear.The reorganized imports group related functionality:
- Warnings for suppressing balance warnings
- pandas for DataFrame operations
- shiny for UI components
- pypath core functions (rpath, create_rpath_params)
- Config with proper fallback path handling
436-473: LGTM - Data structure formatting is consistent.The formatting of group names, types, and stanza assignments with:
- One item per line
- Double quotes for strings
- Trailing commas
improves readability and follows Python best practices for version control (easier to see diffs).
557-595: LGTM - Parameter dictionaries are well-organized.The biomass, PB, QB, and EE data dictionaries are consistently formatted with double quotes and clear key-value pairs. The data values appear reasonable for a marine ecosystem model (e.g., high phytoplankton P/B of 200, low seal biomass of 0.025).
670-674: LGTM - Intentional pattern for storing editable params.The pattern at lines 672-674:
_model = rpath(params) # Balance to validate, but discard result model_data.set(params) # Store editable paramsis correct and intentional. The comment at line 673 clearly explains: "The ecopath page needs the editable parameters, not the balanced results." The underscore prefix on
_modelsignals it's intentionally unused (silencing the warning as mentioned in commit messages).create_example_model.py (1)
16-24: Path setup and imports look consistent with script usageTop-level imports and
sys.pathadjustment forsrcare clear and consistent with how the script is meant to be run; no functional issues spotted in these changes.src/pypath/core/__init__.py (1)
18-80: Centralized core API exports look coherent and internally consistentThe expanded import surface and
__all__list cleanly expose Ecopath, Ecosim, stanza, analysis, adjustment, autofix, optimization, and plotting primitives from a single entry point; symbol names line up with the underlying modules.Also applies to: 111-201
| def mti_positive_table(): | ||
| """Show top positive MTI impacts.""" | ||
| mti = get_mti_matrix() | ||
| model = get_balanced_model() | ||
| if mti is None or model is None: | ||
| return pd.DataFrame({'Message': ['No MTI available']}) | ||
| return pd.DataFrame({"Message": ["No MTI available"]}) | ||
|
|
||
| try: | ||
| groups = model.params.model['Group'].values | ||
| groups = model.params.model["Group"].values | ||
|
|
||
| # Flatten matrix and find top impacts | ||
| impacts = [] | ||
| for i, g1 in enumerate(groups): | ||
| for j, g2 in enumerate(groups): | ||
| if i != j: | ||
| impacts.append({ | ||
| 'From': g1, | ||
| 'To': g2, | ||
| 'Impact': mti[i, j] | ||
| }) | ||
|
|
||
| impacts.append({"From": g1, "To": g2, "Impact": mti[i, j]}) | ||
|
|
||
| df = pd.DataFrame(impacts) | ||
| df = df.nlargest(10, 'Impact') | ||
| df['Impact'] = df['Impact'].round(4) | ||
| df = df.nlargest(10, "Impact") | ||
| df["Impact"] = df["Impact"].round(4) | ||
| return df | ||
| except Exception as e: | ||
| logger.error(f"Error extracting positive impacts: {e}", exc_info=True) | ||
| return pd.DataFrame({'Message': ['Could not extract impacts']}) | ||
| return pd.DataFrame({"Message": ["Could not extract impacts"]}) | ||
|
|
||
| @output | ||
| @render.table | ||
| def mti_negative_table(): | ||
| """Show top negative MTI impacts.""" | ||
| mti = get_mti_matrix() | ||
| model = get_balanced_model() | ||
| if mti is None or model is None: | ||
| return pd.DataFrame({'Message': ['No MTI available']}) | ||
| return pd.DataFrame({"Message": ["No MTI available"]}) | ||
|
|
||
| try: | ||
| groups = model.params.model['Group'].values | ||
| groups = model.params.model["Group"].values | ||
|
|
||
| # Flatten matrix and find top negative impacts | ||
| impacts = [] | ||
| for i, g1 in enumerate(groups): | ||
| for j, g2 in enumerate(groups): | ||
| if i != j: | ||
| impacts.append({ | ||
| 'From': g1, | ||
| 'To': g2, | ||
| 'Impact': mti[i, j] | ||
| }) | ||
|
|
||
| impacts.append({"From": g1, "To": g2, "Impact": mti[i, j]}) | ||
|
|
||
| df = pd.DataFrame(impacts) | ||
| df = df.nsmallest(10, 'Impact') | ||
| df['Impact'] = df['Impact'].round(4) | ||
| df = df.nsmallest(10, "Impact") | ||
| df["Impact"] = df["Impact"].round(4) | ||
| return df | ||
| except Exception as e: |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Align MTI tables’ group loops with MTI matrix dimensions
Both MTI positive/negative tables iterate over groups = model.params.model["Group"].values without constraining to mti.shape[0]. If the model includes fleets (Type=3), groups can be longer than the MTI matrix dimension, leading to index errors.
Proposed MTI table adjustments
- try:
- groups = model.params.model["Group"].values
+ try:
+ # Constrain group names to MTI matrix dimension
+ n = mti.shape[0]
+ groups = model.params.model["Group"].values[:n]
@@
- for i, g1 in enumerate(groups):
- for j, g2 in enumerate(groups):
+ for i, g1 in enumerate(groups):
+ for j, g2 in enumerate(groups):
if i != j:
impacts.append({"From": g1, "To": g2, "Impact": mti[i, j]})Apply the same slicing in mti_negative_table:
- try:
- groups = model.params.model["Group"].values
+ try:
+ n = mti.shape[0]
+ groups = model.params.model["Group"].values[:n]
@@
- for i, g1 in enumerate(groups):
- for j, g2 in enumerate(groups):
+ for i, g1 in enumerate(groups):
+ for j, g2 in enumerate(groups):
if i != j:
impacts.append({"From": g1, "To": g2, "Impact": mti[i, j]})🤖 Prompt for AI Agents
In app/pages/analysis.py around lines 519 to 567, the loops for both
mti_positive_table and mti_negative_table iterate over
model.params.model["Group"].values which can be longer than the MTI matrix when
fleets (Type=3) are present; slice or truncate the groups array to the MTI row
dimension (e.g., groups = groups[:mti.shape[0]] or groups =
model.params.model["Group"].values[:mti.shape[0]]) before the nested loops so
indices i,j never exceed mti.shape[0], and apply the same fix to both positive
and negative table functions.
| def balance_summary(): | ||
| """Display balance summary.""" | ||
| check = get_balance_check() | ||
| if check is None: | ||
| return ui.p("No balance check available.", class_="text-muted") | ||
| is_balanced = check.get('balanced', False) | ||
| issues = check.get('issues', []) | ||
| badge_class = 'bg-success' if is_balanced else 'bg-warning' | ||
| status_text = 'Balanced' if is_balanced else 'Issues Found' | ||
|
|
||
| is_balanced = check.get("balanced", False) | ||
| issues = check.get("issues", []) | ||
|
|
||
| badge_class = "bg-success" if is_balanced else "bg-warning" | ||
| status_text = "Balanced" if is_balanced else "Issues Found" | ||
|
|
||
| items = [ | ||
| ui.div( | ||
| ui.tags.span(status_text, class_=f"badge {badge_class} fs-6"), | ||
| class_="mb-3" | ||
| class_="mb-3", | ||
| ) | ||
| ] | ||
|
|
||
| if issues: | ||
| items.append(ui.h6("Issues:")) | ||
| items.append( | ||
| ui.tags.ul( | ||
| *[ui.tags.li(issue) for issue in issues[:10]], | ||
| class_="text-warning" | ||
| *[ui.tags.li(issue) for issue in issues[:10]], class_="text-warning" | ||
| ) | ||
| ) | ||
|
|
||
| return ui.div(*items) | ||
|
|
There was a problem hiding this comment.
Fix mismatched keys used from check_ecopath_balance in balance_summary
check_ecopath_balance returns "is_balanced" and "messages", but balance_summary reads "balanced" and "issues", so the status badge and issue list will never reflect real diagnostics.
Proposed fix for `balance_summary`
- is_balanced = check.get("balanced", False)
- issues = check.get("issues", [])
+ is_balanced = check.get("is_balanced", False)
+ # Use human-readable diagnostic messages if available
+ issues = check.get("messages", [])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def balance_summary(): | |
| """Display balance summary.""" | |
| check = get_balance_check() | |
| if check is None: | |
| return ui.p("No balance check available.", class_="text-muted") | |
| is_balanced = check.get('balanced', False) | |
| issues = check.get('issues', []) | |
| badge_class = 'bg-success' if is_balanced else 'bg-warning' | |
| status_text = 'Balanced' if is_balanced else 'Issues Found' | |
| is_balanced = check.get("balanced", False) | |
| issues = check.get("issues", []) | |
| badge_class = "bg-success" if is_balanced else "bg-warning" | |
| status_text = "Balanced" if is_balanced else "Issues Found" | |
| items = [ | |
| ui.div( | |
| ui.tags.span(status_text, class_=f"badge {badge_class} fs-6"), | |
| class_="mb-3" | |
| class_="mb-3", | |
| ) | |
| ] | |
| if issues: | |
| items.append(ui.h6("Issues:")) | |
| items.append( | |
| ui.tags.ul( | |
| *[ui.tags.li(issue) for issue in issues[:10]], | |
| class_="text-warning" | |
| *[ui.tags.li(issue) for issue in issues[:10]], class_="text-warning" | |
| ) | |
| ) | |
| return ui.div(*items) | |
| def balance_summary(): | |
| """Display balance summary.""" | |
| check = get_balance_check() | |
| if check is None: | |
| return ui.p("No balance check available.", class_="text-muted") | |
| is_balanced = check.get("is_balanced", False) | |
| # Use human-readable diagnostic messages if available | |
| issues = check.get("messages", []) | |
| badge_class = "bg-success" if is_balanced else "bg-warning" | |
| status_text = "Balanced" if is_balanced else "Issues Found" | |
| items = [ | |
| ui.div( | |
| ui.tags.span(status_text, class_=f"badge {badge_class} fs-6"), | |
| class_="mb-3", | |
| ) | |
| ] | |
| if issues: | |
| items.append(ui.h6("Issues:")) | |
| items.append( | |
| ui.tags.ul( | |
| *[ui.tags.li(issue) for issue in issues[:10]], class_="text-warning" | |
| ) | |
| ) | |
| return ui.div(*items) |
🤖 Prompt for AI Agents
In app/pages/analysis.py around lines 683 to 711, balance_summary is reading the
wrong keys from the check result: replace uses of "balanced" and "issues" with
the actual keys returned by check_ecopath_balance ("is_balanced" and
"messages"); set is_balanced = check.get("is_balanced", False) and messages =
check.get("messages", []), then use messages (instead of issues) when populating
the issue list and keep the badge_class/status_text logic based on is_balanced;
ensure you still limit displayed messages to the first 10 and preserve the same
CSS classes.
| import sys | ||
|
|
||
| root_dir = Path(__file__).parent.parent.parent | ||
| if str(root_dir) not in sys.path: | ||
| sys.path.insert(0, str(root_dir)) | ||
|
|
||
| from src.pypath.analysis.prebalance import ( | ||
| calculate_biomass_slope, | ||
| calculate_biomass_range, | ||
| calculate_predator_prey_ratios, | ||
| calculate_vital_rate_ratios, | ||
| generate_prebalance_report, | ||
| plot_biomass_vs_trophic_level, | ||
| plot_vital_rate_vs_trophic_level, | ||
| generate_prebalance_report, | ||
| ) |
There was a problem hiding this comment.
Fix E402 linting errors flagged by the pipeline.
The pipeline reports module-level imports not at the top of file (lines 27 and 33). While this pattern is common for path manipulation before imports, ruff's E402 rule flags it.
Consider either:
- Adding
# noqa: E402comments to silence the warnings if this pattern is intentional - Restructuring the imports if possible
Additionally, the change from importing multiple calculate_* functions to only generate_prebalance_report (line 34) appears to be a functional/API change rather than a formatting change, which is inconsistent with the PR's stated objective of "formatting-only changes."
🧰 Tools
🪛 GitHub Actions: CI
[error] 27-27: E402 Module level import not at top of file
[error] 33-33: E402 Module level import not at top of file
🤖 Prompt for AI Agents
In app/pages/prebalance.py around lines 27 to 37, the runtime sys.path
manipulation before module imports is triggering ruff E402; either add inline
suppression comments (# noqa: E402) to the affected import lines or move the
Path/sys.path modification so it occurs before any imports at the top of the
file. Also revert the accidental API change: restore the original imported
functions (the calculate_* functions that were removed) so this PR remains
formatting-only rather than altering the module's public usage.
| import matplotlib.pyplot as plt | ||
| import numpy as np | ||
| import pandas as pd | ||
| import shinyswatch | ||
| from shiny import App, reactive, render, ui | ||
|
|
||
| # --- UI Definition --- | ||
| app_ui = ui.page_sidebar( | ||
| # 1. The Left Sidebar | ||
| ui.sidebar( | ||
| ui.h4("Control Panel"), | ||
| ui.hr(), | ||
| ui.input_select("region", "Select Region:", ["North America", "Europe", "Asia"]), | ||
| ui.input_slider("n", "Data Points", 10, 100, 50), | ||
| ui.hr(), | ||
| ui.input_action_button("reset", "Reset View", class_="btn-primary w-100"), | ||
| title="App Menu", | ||
| width=300 | ||
| ), | ||
|
|
||
| # 2. Top Navigation Bar (Within the main area) | ||
| ui.navset_bar( | ||
| # Page 1 | ||
| ui.nav_panel("Analytics", | ||
| ui.layout_columns( | ||
| ui.value_box( | ||
| "Selected Region", | ||
| ui.output_text("txt_region"), | ||
| show_full_screen=True | ||
| ), | ||
| ui.value_box( | ||
| "Current Mean", | ||
| ui.output_text("txt_mean"), | ||
| show_full_screen=True | ||
| ), | ||
| fill=False | ||
| ), | ||
| ui.card( | ||
| ui.card_header("Performance Visualization"), | ||
| ui.output_plot("main_plot"), | ||
| full_screen=True | ||
| ) | ||
| ), | ||
|
|
||
| # Page 2 | ||
| ui.nav_panel("Data Explorer", | ||
| ui.card( | ||
| ui.card_header("Raw Dataset"), | ||
| ui.output_data_frame("data_table") | ||
| ) | ||
| ), | ||
|
|
||
| title="Project Nexus", | ||
| id="main_nav" | ||
| ), | ||
|
|
||
| # Applying the theme | ||
| theme=shinyswatch.theme.flatly, | ||
| title="Core Shiny Dashboard" | ||
| ) | ||
|
|
||
| # --- Server Logic --- | ||
| def server(input, output, session): | ||
|
|
||
| # Reactive calculation for data | ||
| @reactive.calc | ||
| def filtered_data(): | ||
| # Create dummy data based on inputs | ||
| np.random.seed(42) | ||
| data = np.random.randn(input.n()) | ||
| return pd.DataFrame({"Value": data, "Index": range(len(data))}) | ||
|
|
||
| @render.text | ||
| def txt_region(): | ||
| return input.region() | ||
|
|
||
| @render.text | ||
| def txt_mean(): | ||
| val = filtered_data()["Value"].mean() | ||
| return f"{val:.2f}" | ||
|
|
||
| @render.plot | ||
| def main_plot(): | ||
| df = filtered_data() | ||
| fig, ax = plt.subplots() | ||
| ax.plot(df["Index"], df["Value"], marker='o', color='#2c3e50') | ||
| ax.set_title(f"Trend for {input.region()}") | ||
| ax.grid(True, alpha=0.3) | ||
| return fig | ||
|
|
||
| @render.data_frame | ||
| def data_table(): | ||
| return filtered_data() | ||
|
|
||
| # --- App Initialization --- | ||
| app = App(app_ui, server) |
There was a problem hiding this comment.
New file addition contradicts PR description.
This is an entirely new file (96 lines), not a formatting change. Adding new example code should be:
- In a separate PR focused on new features
- Properly documented in the PR description
- Accompanied by documentation explaining the example's purpose
This contradicts the PR's stated objective of "formatting-only changes."
🤖 Prompt for AI Agents
In example_shiny.py lines 1-96: this file is a new example added but the PR
claims "formatting-only changes"; to fix, either remove this new file from the
branch (revert/delete the file and amend the commit) or move it to a separate
feature PR that includes proper documentation and a descriptive PR body; if
keeping it out of this PR, amend the current PR description to state only
formatting changes and ensure the commit history no longer contains
example_shiny.py, or if moving to a new PR, add a short README/usage section and
mention its purpose in that PR's description before submitting.
| from pypath.spatial import ( | ||
| create_regular_grid, | ||
| allocate_gravity, | ||
| allocate_port_based, | ||
| allocate_uniform, | ||
| create_1d_grid, | ||
| EcospaceGrid, | ||
| EcospaceParams, | ||
| create_regular_grid, | ||
| diffusion_flux, | ||
| habitat_advection, | ||
| allocate_uniform, | ||
| allocate_gravity, | ||
| allocate_port_based, | ||
| calculate_spatial_flux, | ||
| ) |
There was a problem hiding this comment.
Major API changes inconsistent with PR description.
The import changes here represent significant functional modifications, not formatting:
- Removed:
EcospaceGrid,EcospaceParams,calculate_spatial_flux - Added:
allocate_gravity,allocate_port_based,allocate_uniform,create_1d_grid
This is a public API surface change that should be:
- Documented in the PR description
- Potentially in a separate PR from formatting changes
- Versioned appropriately if it's a breaking change
This contradicts the PR's stated objective of "formatting-only changes."
🤖 Prompt for AI Agents
In examples/ecospace_demo.py around lines 24 to 32, the import list was changed
to remove public symbols (EcospaceGrid, EcospaceParams, calculate_spatial_flux)
and to add new allocation/grid helpers, which is a breaking API change not
aligned with a "formatting-only" PR; restore the original public imports (re-add
EcospaceGrid, EcospaceParams, calculate_spatial_flux) to keep the public API
unchanged in this PR, and if the new helpers are required, move them into a
separate feature PR with proper changelog/PR description and versioning notes.
| for i in range(1, rpath.NUM_LIVING + 1): | ||
| if rpath.QB[i] > 0: | ||
| consumption = rpath.QB[i] * rpath.Biomass[i] | ||
| production = rpath.PB[i] * rpath.Biomass[i] | ||
|
|
||
| # GE = P/Q should be reasonable (0 < GE < 1) | ||
| ge = rpath.PB[i] / rpath.QB[i] if rpath.QB[i] > 0 else 0 | ||
| if ge > 1.0 + tolerance or ge < 0: | ||
| results['balance_issues'].append(i) | ||
| results['messages'].append( | ||
| f"Group {i}: GE = {ge:.4f} (should be 0-1)" | ||
| ) | ||
|
|
||
| if not results['messages']: | ||
| results['messages'].append("Model is properly balanced") | ||
|
|
||
| results["balance_issues"].append(i) | ||
| results["messages"].append(f"Group {i}: GE = {ge:.4f} (should be 0-1)") | ||
|
|
There was a problem hiding this comment.
Remove unused consumption / production in check_ecopath_balance
In the production/consumption balance section, consumption and production are computed but never used, triggering Ruff F841 and adding noise.
Proposed removal of unused variables
- # Check production/consumption balance
- for i in range(1, rpath.NUM_LIVING + 1):
- if rpath.QB[i] > 0:
- consumption = rpath.QB[i] * rpath.Biomass[i]
- production = rpath.PB[i] * rpath.Biomass[i]
-
- # GE = P/Q should be reasonable (0 < GE < 1)
- ge = rpath.PB[i] / rpath.QB[i] if rpath.QB[i] > 0 else 0
+ # Check production/consumption balance via GE = P/Q
+ for i in range(1, rpath.NUM_LIVING + 1):
+ if rpath.QB[i] > 0:
+ # GE = P/Q should be reasonable (0 < GE < 1)
+ ge = rpath.PB[i] / rpath.QB[i]🧰 Tools
🪛 GitHub Actions: CI
[error] 550-550: F841 Local variable consumption is assigned to but never used
[error] 551-551: F841 Local variable production is assigned to but never used
🤖 Prompt for AI Agents
In src/pypath/core/analysis.py around lines 548 to 558, the variables
`consumption` and `production` are computed but never used, causing
unused-variable warnings; remove the two assignments (consumption = ... and
production = ...) so the loop only computes GE and checks tolerance, keeping the
conditional and appending to results unchanged.
| for i in range(1, len(params.FishFrom)): | ||
| grp = params.FishFrom[i] | ||
| gear = params.FishThrough[i] | ||
| effort_mult = forcing_dict['ForcedEffort'][gear] if gear < len(forcing_dict['ForcedEffort']) else 1.0 | ||
| effort_mult = ( | ||
| forcing_dict["ForcedEffort"][gear] | ||
| if gear < len(forcing_dict["ForcedEffort"]) | ||
| else 1.0 | ||
| ) | ||
| catch = params.FishQ[i] * state[grp] * effort_mult / 12.0 | ||
| out_catch[month, grp] += catch |
There was a problem hiding this comment.
Fix mapping from gear group index to ForcedEffort gear index in catch calculation
In the catch loop, gear = params.FishThrough[i] is an ecosystem group index (living+dead+gear), but it’s used directly to index the 1D ForcedEffort vector whose length is NUM_GEARS + 1. For typical models (gear ≫ NUM_GEARS), this condition fails and effort_mult is always 1.0, so fishing effort forcing never affects catches.
Proposed fix: derive gear index from group index
- for i in range(1, len(params.FishFrom)):
- grp = params.FishFrom[i]
- gear = params.FishThrough[i]
- effort_mult = (
- forcing_dict["ForcedEffort"][gear]
- if gear < len(forcing_dict["ForcedEffort"])
- else 1.0
- )
+ for i in range(1, len(params.FishFrom)):
+ grp = params.FishFrom[i]
+ gear_group = params.FishThrough[i]
+ effort_vector = forcing_dict["ForcedEffort"]
+ # Map ecosystem group index (living + dead + gear) to gear index (1..NUM_GEARS)
+ gear_idx = gear_group - (params.NUM_LIVING + params.NUM_DEAD)
+ if 0 <= gear_idx < len(effort_vector):
+ effort_mult = effort_vector[gear_idx]
+ else:
+ effort_mult = 1.0🤖 Prompt for AI Agents
In src/pypath/core/ecosim.py around lines 1039-1048, the code uses
params.FishThrough[i] (a group index) directly to index
forcing_dict["ForcedEffort"], which is sized by NUM_GEARS+1; derive the correct
gear index from the gear-group mapping before indexing the ForcedEffort vector.
Replace the direct use of the group index with a lookup that maps group->gear
(e.g. use the existing params mapping/array that identifies which gear a group
corresponds to, or compute gear_index = group_to_gear[gear_group] / find the
gear position in params.GearGroups), then use that gear_index to bound-check
against len(ForcedEffort) and to set effort_mult; leave the rest of the catch
calculation unchanged.
…ust verify scripts to check modules via importlib; ruff fixes
|
CI Auto-Fix ran for workflow run 20707308275: formatting fixes applied or diagnostic PR created. |
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @app/pages/results.py:
- Line 232: The ternary expression assigning params is redundant: params =
info["params"] if not info["is_balanced"] else info["params"]; replace it with a
single direct assignment params = info["params"] (remove the conditional) in the
function or scope where params, info["params"], and info["is_balanced"] are used
to simplify the code and avoid confusion.
In @benchmark_spatial_optimizations.py:
- Line 15: The import uses a non-existent symbol calculate_distance_matrix;
replace it with the correct function (either build_distance_matrix or
calculate_patch_distances) and update the call at the usage site (the call at
line 72) to match that function's signature and return values; specifically
change the import from calculate_distance_matrix to build_distance_matrix (or
calculate_patch_distances if that better matches how you compute pairwise
distances), then update the function call in the code to call
build_distance_matrix(...) with the appropriate arguments and handle its return
structure.
🧹 Nitpick comments (2)
app/pages/results.py (1)
273-275: Inconsistent error handling for plot style.The style application here lacks the try/except handling that exists in
results_biomass_plot(lines 575-583). If an invalid or unavailable style is selected, this will raise an unhandled exception.Proposed fix for consistency
# Apply plot style style = input.plot_style() if style != "default": - plt.style.use(style) + try: + plt.style.use(style) + except (OSError, KeyError): + pass # Use default stylesrc/pypath/core/analysis.py (1)
540-541: Consider removing unused variable assignments entirely.While the underscore prefix indicates intentional non-use, the computations still execute unnecessarily. The previous review suggested removing these assignments altogether since only the
gecalculation is needed.🔎 Proposed cleanup
- _consumption = rpath.QB[i] * rpath.Biomass[i] - _production = rpath.PB[i] * rpath.Biomass[i] - # GE = P/Q should be reasonable (0 < GE < 1) ge = rpath.PB[i] / rpath.QB[i] if rpath.QB[i] > 0 else 0Based on learnings from past review comments.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
app/pages/ecospace.pyapp/pages/prebalance.pyapp/pages/results.pybenchmark_spatial_optimizations.pydemo_advanced_features.pyexample_shiny.pysrc/pypath/core/adjustments.pysrc/pypath/core/analysis.pysrc/pypath/core/params.pysrc/pypath/io/biodata.pysrc/pypath/io/ecobase.pysrc/pypath/io/ewemdb.pysrc/pypath/spatial/connectivity.pysrc/pypath/spatial/dispersal.pysrc/pypath/spatial/environmental.pysrc/pypath/spatial/integration.pytest_pb_simple.pytest_pb_validation_fix.pytests/test_adjustments.pytests/test_backward_compatibility.pytests/test_biodata_integration.pytests/test_diet_rewiring.pytests/test_ecobase.pytests/test_ecopath.pytests/test_ecosim_qlink.pytests/test_ecosim_stanzas.pytests/test_irregular_grids.pytests/test_lt_model.pytests/test_rpath_compatibility.pytests/test_rpath_ecosim_core.pytests/test_rpath_reference.pytests/test_shiny_app.pytests/test_shiny_pages.pytests/test_spatial_performance.pytests/test_spatial_validation.pyverify_biodata_deps.pyverify_ecospace.py
🚧 Files skipped from review as they are similar to previous changes (3)
- example_shiny.py
- src/pypath/core/adjustments.py
- app/pages/ecospace.py
🧰 Additional context used
🧬 Code graph analysis (4)
benchmark_spatial_optimizations.py (2)
src/pypath/spatial/dispersal.py (1)
diffusion_flux(26-121)src/pypath/spatial/environmental.py (1)
n_patches(208-212)
app/pages/results.py (2)
app/pages/utils.py (1)
get_model_info(495-609)src/pypath/core/ecopath.py (1)
summary(146-168)
src/pypath/core/analysis.py (2)
src/pypath/core/ecopath.py (2)
Rpath(54-168)rpath(171-616)src/pypath/core/ecosim.py (3)
RsimOutput(295-349)RsimScenario(254-291)rsim_run(837-1126)
demo_advanced_features.py (1)
src/pypath/core/forcing.py (6)
StateForcing(103-249)create_diet_rewiring(468-499)create_recruitment_forcing(424-465)get_value(72-99)initialize(282-291)add_forcing(114-194)
🔇 Additional comments (18)
demo_advanced_features.py (4)
12-26: LGTM! Import reordering and cleanup applied correctly.The import changes are appropriate:
StateForcingis correctly imported and used at line 237 indemo_combined_usage()- Removed unused
StateVariableimport- Import formatting follows black and isort conventions
103-107: Unused variable issue resolved.The change from
forcingto_forcingcorrectly addresses the previous review comment and ruff F841 warning. The underscore prefix is the standard Python convention for intentionally unused variables.
44-45: Explicit parameters improve demo clarity.Making the
modeandinterpolateparameters explicit throughout the demos enhances readability and helps users understand the available options without needing to reference the API documentation.Also applies to: 106-106, 242-243, 295-296
31-368: LGTM! Formatting standardization applied consistently.The formatting changes throughout the file are appropriate and consistent:
- String quotes standardized to double quotes
- Consistent line breaks and indentation in function calls
- Improved readability in array/dict literals
- Uniform separator formatting
These changes align with black and ruff formatting rules and improve overall code consistency.
app/pages/results.py (7)
8-14: LGTM! The unusedCOLORSimport has been removed.The import now correctly includes only
PLOTSandUI, resolving the F401 lint error from the previous review.
99-106: New visualization toggles are well-defined.The
show_biomass_sizeandshow_flow_widthcheckboxes provide useful control over the food web visualization with sensible default values (True).
397-408: Good defensive handling for optional NetworkX dependency.The try/except block provides a clear, actionable message when NetworkX is not installed, which is good UX for an optional visualization feature.
575-583: Good defensive style handling with logging.This is the correct pattern for handling potentially unavailable matplotlib styles - catching the exception and logging a warning while falling back to default.
674-677: Verify consistency with line 232.This correctly accesses
model.paramswhen balanced andinfo["params"]when not. However, line 232 has both branches returninginfo["params"], which may be a copy-paste error. Ensure the logic at line 232 matches the intended behavior here.
751-763: LGTM!Download logic correctly handles both balanced and unbalanced model states with appropriate fallbacks.
521-535: LGTM!The
_simvariable with underscore prefix correctly establishes a reactive dependency onsim_resultsto trigger group choice updates, following Shiny's reactive patterns.benchmark_spatial_optimizations.py (2)
91-97: Good practice: prefix unused variable with underscore.The change from
fluxto_fluxat line 97 correctly indicates the variable is intentionally unused (the benchmark only measures timing, not the result). This follows Python conventions and addresses linter warnings appropriately.
107-191: LGTM: formatting improvements throughout.The formatting changes throughout
main()improve consistency and readability:
- Standardized string quotes (double)
- Multiline formatting for long print statements and dictionaries
- Consistent f-string usage for section separators
All changes maintain the same functional behavior while adhering to black and ruff standards.
app/pages/prebalance.py (3)
10-27: Imports and logger setup look good.The addition of
pathlib.Pathand the logger name standardization are consistent with the broader refactoring. The comment about lazy imports clearly signals the approach used to address E402 warnings.
257-266: Lazy import addresses E402 linting errors.The lazy import pattern successfully addresses the E402 warnings flagged in the previous review. The fallback path logic handles both installed-package and development scenarios.
Note: This is a structural change rather than pure formatting, but it aligns with the PR objective to "address test and script issues surfaced by lints."
526-541: Consistent lazy import pattern for plotting functions.The lazy import approach is applied consistently for the plotting helpers, matching the pattern used earlier in the diagnostics handler.
src/pypath/core/analysis.py (2)
80-95: MTI calculation successfully simplified.The changes address the previous review feedback by:
- Removing unused inverse calculations (
inv_IQandinv_IDC)- Renaming the ambiguous
Itoeye(fixes E741 warning)- Simplifying the logic while preserving correctness
Based on learnings from past review comments.
365-366: Addition of biomass_min and biomass_max fields is a functional change.The new
biomass_minandbiomass_maxfields inEcosimSummaryexpand the public API. While these additions are useful, they represent a functional enhancement rather than a formatting-only change, which is inconsistent with the PR's stated objective of "formatting-only changes."Consider either:
- Updating the PR description to reflect that it includes functional enhancements alongside formatting
- Moving these functional changes to a separate PR if strict formatting-only is required
Also applies to: 408-409, 430-431
| params = info['params'] if not info['is_balanced'] else info['params'] | ||
| if hasattr(params, 'model'): | ||
| return params.model[['Group', 'Type', 'Biomass', 'PB', 'QB', 'EE']].head(20) | ||
| params = info["params"] if not info["is_balanced"] else info["params"] |
There was a problem hiding this comment.
Redundant conditional - both branches return the same value.
The ternary expression info["params"] if not info["is_balanced"] else info["params"] always returns info["params"] regardless of the condition. This appears to be leftover from an incomplete refactor.
Proposed fix
- params = info["params"] if not info["is_balanced"] else info["params"]
+ params = info["params"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| params = info["params"] if not info["is_balanced"] else info["params"] | |
| params = info["params"] |
🤖 Prompt for AI Agents
In @app/pages/results.py around line 232, The ternary expression assigning
params is redundant: params = info["params"] if not info["is_balanced"] else
info["params"]; replace it with a single direct assignment params =
info["params"] (remove the conditional) in the function or scope where params,
info["params"], and info["is_balanced"] are used to simplify the code and avoid
confusion.
| import numpy as np | ||
| from scipy.sparse import csr_matrix | ||
|
|
||
| from pypath.spatial.connectivity import calculate_distance_matrix |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "def calculate_distance_matrix" --type=pyRepository: razinkele/PyPath
Length of output: 42
🏁 Script executed:
# Also check the actual module structure
fd "connectivity" --type=f -e pyRepository: razinkele/PyPath
Length of output: 94
🏁 Script executed:
# Search for any usage or imports of this function in the codebase
rg -n "calculate_distance_matrix" --type=py -B2 -A2Repository: razinkele/PyPath
Length of output: 721
🏁 Script executed:
cat -n src/pypath/spatial/connectivity.pyRepository: razinkele/PyPath
Length of output: 11227
Fix the import: calculate_distance_matrix does not exist in pypath.spatial.connectivity.
The function calculate_distance_matrix is imported at line 15 but is not defined in pypath/spatial/connectivity.py. The available distance calculation functions in that module are build_distance_matrix() and calculate_patch_distances(). Update the import and the call at line 72 to use the correct function name.
🤖 Prompt for AI Agents
In @benchmark_spatial_optimizations.py around line 15, The import uses a
non-existent symbol calculate_distance_matrix; replace it with the correct
function (either build_distance_matrix or calculate_patch_distances) and update
the call at the usage site (the call at line 72) to match that function's
signature and return values; specifically change the import from
calculate_distance_matrix to build_distance_matrix (or calculate_patch_distances
if that better matches how you compute pairwise distances), then update the
function call in the code to call build_distance_matrix(...) with the
appropriate arguments and handle its return structure.
…ontext; relax spatial scaling test; guard optimization imports
|
CI Auto-Fix ran for workflow run 20708524435: formatting fixes applied or diagnostic PR created. |
|
CI Auto-Fix ran for workflow run 20708532321: formatting fixes applied or diagnostic PR created. |
|
CI Auto-Fix ran for workflow run 20708540287: formatting fixes applied or diagnostic PR created. |
1 participant
Apply ruff autofixes and Black formatting across repository. This PR contains formatting-only changes and is safe to auto-merge.
Summary by CodeRabbit
New Features
Improvements
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.