-
-
Notifications
You must be signed in to change notification settings - Fork 228
ENH: Add JSON export and CSV export support for monte carlo result #916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #916 +/- ##
===========================================
+ Coverage 80.27% 81.60% +1.32%
===========================================
Files 104 107 +3
Lines 12769 13854 +1085
===========================================
+ Hits 10250 11305 +1055
- Misses 2519 2549 +30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds JSON export functionality to the MonteCarlo class, allowing users to export simulation results to a JSON file. However, the implementation contains several critical bugs that will prevent it from working correctly, along with style inconsistencies and test organization issues.
Key Changes
- Added
export_json()method to theMonteCarloclass for exporting results to JSON format - Created test file for validating JSON export functionality
- Included test data files (which should not be committed)
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| rocketpy/simulation/monte_carlo.py | Implements new export_json method with parameter validation, data conversion, and file writing logic |
| tests/test_monte_carlo_export.py | Adds basic test for JSON export functionality |
| temp_test_output.json | Test output artifact - should not be committed |
| monte_carlo_test.outputs.txt | Test output artifact - should not be committed |
| monte_carlo_test.inputs.txt | Test input artifact - should not be committed |
| monte_carlo_test.errors.txt | Empty test error file - should not be committed |
|
this PR is duplicated with the #912 |
| def test_json_export(monte_carlo_calisto): | ||
| mc = monte_carlo_calisto | ||
| mc.simulate(3) | ||
|
|
||
| filename = "temp_test_output.json" | ||
| """ | ||
| tests weather the results of monte carlo are exported to a JSON file. | ||
| """ | ||
| mc.export_json(filename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| def test_json_export(monte_carlo_calisto): | |
| mc = monte_carlo_calisto | |
| mc.simulate(3) | |
| filename = "temp_test_output.json" | |
| """ | |
| tests weather the results of monte carlo are exported to a JSON file. | |
| """ | |
| mc.export_json(filename) | |
| def test_json_export(monte_carlo_calisto): | |
| """ | |
| tests weather the results of monte carlo are exported to a JSON file. | |
| """ | |
| mc = monte_carlo_calisto | |
| mc.simulate(3) | |
| filename = "temp_test_output.json" | |
| mc.export_json(filename) |
| finally: | ||
| if os.path.exists(filename): | ||
| os.remove(filename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be more pythonic...
finally:
if os.path.exists(filename):
os.remove(filename)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you dont need to create the tests/integration/simulation/test_monte_carlo_export_csv_create_valid_file.py and tests/integration/simulation/test_monte_carlo_export_json_creates_valid_file.py files, there are only 2 est functions, which could be easily placed at the tests/unit/simulation/flight.py file (or similar)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether we should create a "MonteCarloDataExporter" similarly to what we have recelty done with the Flight class.
Needs to think more about it.
Gui-FernandesBR
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation is clean enough, seems good. But I will compare it against the other PR which I mentioned as duplicated PR
Code changes (bugfix, features)
Lint (
black rocketpy/ tests/) has passed locallyCurrent behavior
No feature to export result in the form of JSON or CSV file
New behavior
added a feature to export the result of monte carlo simulation into a JSON file and CSV file
Breaking change
No
Additional information