Fix critical bugs and enhance model coverage in generate_model_size_report.py script#89
Conversation
Co-authored-by: bernalde <8647329+bernalde@users.noreply.github.com>
…ck addressed Co-authored-by: bernalde <8647329+bernalde@users.noreply.github.com>
|
Tests are failing. Aim to merge this with main branch. Consider further cvopilot feedback and original goals in #86 |
Co-authored-by: bernalde <8647329+bernalde@users.noreply.github.com>
Fixed the failing tests by applying Black code formatting and made the script compatible with main branch. The script now: ✅ Passes all linting checks All original bug fixes from the Copilot feedback are preserved. Ready for merge with main branch. Commit: 7319f58 |
|
Have the table be generated for all the models. |
bernalde
left a comment
There was a problem hiding this comment.
Modify the README file to add links to each model as before.
|
@bernalde Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot try again |
…on table Co-authored-by: bernalde <8647329+bernalde@users.noreply.github.com>
|
I've successfully implemented both requested changes:
The generated table now includes comprehensive size metrics for all successfully processed models with direct links to their respective directories, just like the original "Available Models" section format. Commit: 39a1da6 |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses critical bugs in the generate_model_size_report.py script and significantly expands its functionality to process all GDPlib models. The key fixes include resolving a NameError when no models are processed, eliminating duplicate error handling, and preventing DataFrame mutation issues. The enhancement expands model coverage from 2 to 19 models and adds clickable navigation links to model directories in the generated comparison table.
- Fix critical NameError bug when all models fail to process by properly organizing code within conditional blocks
- Expand model coverage to include all 19 models in GDPlib instead of just 2
- Add clickable directory links to model names in the generated comparison table for improved navigation
Reviewed Changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| generate_model_size_report.py | Fixes critical bugs, expands model coverage, and adds linked headers to comparison table |
| gdplib/*/model_size_report.md | Generated individual model size reports for 16 different models |
| README.md | Updated with comprehensive model size comparison table containing clickable links |
| "stranded_gas", | ||
| "syngas", | ||
| "water_network" | ||
| "water_network", |
There was a problem hiding this comment.
[nitpick] The trailing comma after 'water_network' is inconsistent with the original code style. Since this is the last item in the list, the comma should be removed to maintain consistency with the previous pattern.
| "water_network", | |
| "water_network" |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This PR addresses all the issues identified in Copilot's code review of PR #86, fixing several critical bugs in the
generate_model_size_report.pyscript and expanding its coverage to include all models in GDPlib.Critical Bug Fixes
1. NameError when no models are processed
The script would crash with
NameError: name 'combined_df' is not definedwhen all models failed to process due to missing dependencies or other errors. This was caused by improper indentation where the combined report generation code was outside theelseblock.Before:
After:
2. Duplicate and conflicting error handling
The script had duplicate checks for missing README sections with conflicting behavior - first raising an exception, then handling it gracefully.
3. DataFrame column mutation issue
The same DataFrame object was having its columns modified multiple times, which could lead to unexpected behavior and made the code harder to understand.
Enhanced Model Coverage
All models enabled
Expanded the script to process all 19 models in GDPlib instead of just 2 conservative models, providing comprehensive coverage of the entire library.
Model directory links
Enhanced the generated table to include clickable links to each model directory, improving navigation and documentation quality. Each model name is now formatted as
[model_name](./gdplib/model_name/).Testing Results
The enhanced script now:
model_size_report.mdfiles for each successful modelGenerated Output
The script now successfully generates a comprehensive model size comparison table showing metrics for all successfully processed models with clickable navigation:
All changes maintain backward compatibility while significantly expanding the script's functionality and robustness.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.