-
-
Notifications
You must be signed in to change notification settings - Fork 228
ENH: Add Monte Carlo outputs export to CSV/JSON (#242) #912
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?
Changes from all commits
e51b221
fe7c69e
1c1c77a
ee4bab5
d0cc223
6503273
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import json | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import csv | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import os | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import traceback | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import warnings | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -588,6 +589,41 @@ def __evaluate_flight_outputs(self, flight, sim_idx): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| json.dumps(outputs_dict, cls=RocketPyEncoder, **self._export_config) + "\n" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def export_results(self, output_filename, output_format): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Converts the default Monte Carlo .txt output to .cvs or .json file | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Converts the default Monte Carlo .txt output to .cvs or .json file | |
| """Converts the default Monte Carlo .txt output to .csv or .json file |
Copilot
AI
Dec 9, 2025
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 docstring is missing important details. It should:
- Specify that
output_filenamecan be a string or Path object - Document the valid values for
output_format("csv" or "json") - Explain what happens if the output file already exists (will it be overwritten?)
- Mention that the method reads from
self.filename.outputs.txt - Follow NumPy docstring format with a proper description section before Parameters
Example improvement:
\"\"\"Export Monte Carlo simulation results to CSV or JSON format.
This method reads simulation output data from the `.outputs.txt` file
and converts it to either CSV or JSON format based on user selection.
Parameters
----------
output_filename : str or Path
Path and name of the output file (without extension). The appropriate
extension (.csv or .json) will be added automatically.
output_format : str
Format of the output file. Must be either "csv" or "json" (case-insensitive).
Returns
-------
None
Raises
------
FileNotFoundError
If the `.outputs.txt` file does not exist.
ValueError
If `output_format` is not "csv" or "json".
\"\"\"| """Converts the default Monte Carlo .txt output to .cvs or .json file | |
| depending on the user's choice | |
| Parameters | |
| ---------- | |
| output_filename : str | |
| Name of the file in which the converted data will be saved | |
| output_format : str | |
| Format of the output file | |
| Returns | |
| ------- | |
| None | |
| """Export Monte Carlo simulation results to CSV or JSON format. | |
| This method reads simulation output data from the `.outputs.txt` file | |
| (specifically, from `self.filename.outputs.txt`) and converts it to either | |
| CSV or JSON format based on user selection. The output file will be | |
| overwritten if it already exists. | |
| Parameters | |
| ---------- | |
| output_filename : str or Path | |
| Path and name of the output file (without extension). The appropriate | |
| extension (.csv or .json) will be added automatically. | |
| output_format : str | |
| Format of the output file. Must be either "csv" or "json" | |
| (case-insensitive). | |
| Returns | |
| ------- | |
| None | |
| Raises | |
| ------ | |
| FileNotFoundError | |
| If the `.outputs.txt` file does not exist. | |
| ValueError | |
| If `output_format` is not "csv" or "json". | |
| Examples | |
| -------- | |
| >>> mc.export_results("results", "csv") | |
| >>> mc.export_results(Path("results_folder/results"), "json") |
Copilot
AI
Dec 9, 2025
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.
Missing error handling for file I/O operations. The method should handle FileNotFoundError when the .outputs.txt file doesn't exist and provide a clear error message to the user. Additionally, consider handling json.JSONDecodeError if the file contains malformed JSON data.
| with open(f"{self.filename}.outputs.txt", "r", encoding="utf-8") as f: | |
| for line in f: | |
| line = line.strip() | |
| data = json.loads(line) | |
| txt_data.append(data) | |
| try: | |
| with open(f"{self.filename}.outputs.txt", "r", encoding="utf-8") as f: | |
| for line_number, line in enumerate(f, start=1): | |
| line = line.strip() | |
| try: | |
| data = json.loads(line) | |
| txt_data.append(data) | |
| except json.JSONDecodeError as e: | |
| _SimMonitor.reprint( | |
| f"Error decoding JSON on line {line_number} of '{self.filename}.outputs.txt': {e}\n" | |
| f"Line content: {line}\n" | |
| "Please check the file for malformed JSON data." | |
| ) | |
| raise | |
| except FileNotFoundError: | |
| _SimMonitor.reprint( | |
| f"File '{self.filename}.outputs.txt' not found. " | |
| "Please ensure that the Monte Carlo simulation has been run and the output file exists." | |
| ) | |
| raise |
Copilot
AI
Dec 9, 2025
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.
Potential IndexError if txt_data is empty. If the .outputs.txt file is empty or contains only blank lines, txt_data[0] will raise an IndexError. Add validation to check that txt_data is not empty before attempting to access its first element.
Copilot
AI
Dec 9, 2025
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.
Extra space before empty string in newline= "" parameter. Should be newline="" (no space before the equals sign) to follow Python style conventions (PEP 8).
| with open(f"{output_filename}.csv", "w", newline= "") as f: | |
| with open(f"{output_filename}.csv", "w", newline="") as f: |
Copilot
AI
Dec 9, 2025
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.
Missing input validation for output_format parameter. The method should validate that output_format is either "csv" or "json" and raise a ValueError with a helpful message if an unsupported format is provided. Currently, if an unsupported format is passed, the method silently does nothing, which could confuse users.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,8 @@ | ||||||||||||||||||||||||||
| import matplotlib as plt | ||||||||||||||||||||||||||
| import numpy as np | ||||||||||||||||||||||||||
| import pytest | ||||||||||||||||||||||||||
| import json | ||||||||||||||||||||||||||
| import os | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| from rocketpy.simulation import MonteCarlo | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
@@ -185,3 +187,31 @@ def test_estimate_confidence_interval_raises_type_error_for_invalid_statistic(): | |||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| with pytest.raises(TypeError): | ||||||||||||||||||||||||||
| mc.estimate_confidence_interval("apogee", statistic="not_a_function") | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| def test_monte_carlo_export_results_to_csv_and_json_files(monte_carlo_calisto, tmp_path): | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
| def test_monte_carlo_export_results_to_csv_and_json_files(monte_carlo_calisto, tmp_path): | |
| def test_export_results_creates_csv_and_json_files(monte_carlo_calisto, tmp_path): |
Copilot
AI
Dec 9, 2025
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.
Unnecessary f-string usage. The expression f"{"mock_output_in_json"}" is redundant and can be simplified to just "mock_output_in_json".
| expected_file_in_json = tmp_path / f"{"mock_output_in_json"}.json" | |
| expected_file_in_json = tmp_path / "mock_output_in_json.json" |
Copilot
AI
Dec 9, 2025
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.
Unnecessary f-string usage. The expression f"{"mock_outputs_in_csv"}" is redundant and can be simplified to just "mock_outputs_in_csv". The same applies to line 212.
| expected_file_in_csv = tmp_path / f"{"mock_outputs_in_csv"}.csv" | |
| assert expected_file_in_csv.exists() | |
| mc.export_results(tmp_path / "mock_output_in_json", "json") | |
| expected_file_in_json = tmp_path / f"{"mock_output_in_json"}.json" | |
| expected_file_in_csv = tmp_path / "mock_outputs_in_csv.csv" | |
| assert expected_file_in_csv.exists() | |
| mc.export_results(tmp_path / "mock_output_in_json", "json") | |
| expected_file_in_json = tmp_path / "mock_output_in_json.json" |
Copilot
AI
Dec 9, 2025
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 test only verifies that files are created but doesn't validate their contents. Consider adding assertions to:
- Verify the CSV/JSON files contain the expected mock data (
apogee: 100,max_velocity: 255) - Check CSV header format
- Verify JSON structure and indentation
Example:
# For CSV
import csv
with open(expected_file_in_csv, "r") as f:
reader = csv.DictReader(f)
rows = list(reader)
assert len(rows) == 1
assert rows[0]["apogee"] == "100"
assert rows[0]["max_velocity"] == "255"
# For JSON
with open(expected_file_in_json, "r") as f:
data = json.load(f)
assert len(data) == 1
assert data[0]["apogee"] == 100
assert data[0]["max_velocity"] == 255
Copilot
AI
Dec 9, 2025
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 test doesn't cover important edge cases and error conditions:
- Invalid
output_formatparameter (e.g., "xml", "pdf") - Non-existent
.outputs.txtfile - Empty
.outputs.txtfile - Malformed JSON in
.outputs.txtfile - Case insensitivity of
output_format(e.g., "CSV", "Json")
Consider adding separate test cases for these scenarios to ensure the method handles errors gracefully.
Copilot
AI
Dec 9, 2025
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.
File cleanup in the finally block will fail if the files don't exist. This can happen if the fixture fails to create these files or if the test is run in isolation. Use os.path.exists() checks or use a helper function similar to _post_test_file_cleanup() found in integration tests (see tests/integration/simulation/test_monte_carlo.py:12-25).
Suggested fix:
finally:
for filepath in ["monte_carlo_test.errors.txt", "monte_carlo_test.inputs.txt", "monte_carlo_test.outputs.txt"]:
if os.path.exists(filepath):
os.remove(filepath)| os.remove("monte_carlo_test.errors.txt") | |
| os.remove("monte_carlo_test.inputs.txt") | |
| os.remove("monte_carlo_test.outputs.txt") | |
| for filepath in [ | |
| "monte_carlo_test.errors.txt", | |
| "monte_carlo_test.inputs.txt", | |
| "monte_carlo_test.outputs.txt", | |
| ]: | |
| if os.path.exists(filepath): | |
| os.remove(filepath) |
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.
[nitpick] The documentation could be more informative. Consider adding:
.outputs.txtfileExample improvement: