-
-
Notifications
You must be signed in to change notification settings - Fork 166
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
ENH: Parallel mode for monte-carlo simulations #619
base: develop
Are you sure you want to change the base?
Conversation
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.
Amazing feature, as the results show the MonteCarlo
class has great potential for parallelization.
The only blocking issue I see with this PR is the serialization
code. It still does not support all of rocketpy
features and requires a lot of maintanance and updates on our end.
Do you see any other option for performing the serialization of inputs?
@phmbressan we should make all the classes json serializable, it's an open issue at #522 . In the meantime, maybe we could still use the _encoders module to serialize inputs. I agree with you that implementing flight class serialization within this PR may conflict create maintenance issues for us. The simplest solution would be to delete the |
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.
@phmbressan really good modifications to this PR. Great work.
Before merging, please run 1000 simulations so the example becomes better illustrated on the documentation, please.
I have pushed a fix for the issue on file writing when running on Windows (more accurately on processes Issues solved by this PR:
Points of Improvement:
Some of these points could become issues of the repository. Stating them here for proper PR documentation. Future Considerations:
|
@phmbressan I like the way this PR was refactored. Many thanks for your effort. Please fix the pylint errors and solve all the open conversations in this PR so we can approve and merge it onto develop! Optionally, try to rebase the PR to get the latest commits from develop. |
rocketpy/simulation/monte_carlo.py
Outdated
if n_workers is None or n_workers > os.cpu_count(): | ||
n_workers = os.cpu_count() | ||
|
||
if n_workers < 2: | ||
raise ValueError("Number of workers must be at least 2 for parallel mode.") |
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.
We should print the number of workers being used with _SimMonitor.reprint
here.
|
||
sim_consumer.start() | ||
|
||
for seed in seeds: |
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.
Very minor, but I think the consumer should start after the producers
rocketpy/simulation/monte_carlo.py
Outdated
) | ||
processes.append(sim_producer) | ||
|
||
for sim_producer in processes: | ||
sim_producer.start() | ||
|
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.
There is an extra for loop here
) | |
processes.append(sim_producer) | |
for sim_producer in processes: | |
sim_producer.start() | |
) | |
processes.append(sim_producer) | |
sim_producer.start() | |
rocketpy/simulation/monte_carlo.py
Outdated
while sim_monitor.keep_simulating(): | ||
sim_idx = sim_monitor.increment() - 1 | ||
|
||
self.environment._set_stochastic(seed) | ||
self.rocket._set_stochastic(seed) | ||
self.flight._set_stochastic(seed) | ||
|
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.
Every single iteration needs to be re-seeded?
If this was done before the while loop, wouldn't it be enough?
rocketpy/simulation/monte_carlo.py
Outdated
@@ -253,114 +491,52 @@ def __run_single_simulation(self, input_file, output_file): | |||
] | |||
for item in d.items() | |||
) | |||
inputs_dict["idx"] = sim_idx |
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.
inputs_dict["idx"] = sim_idx | |
inputs_dict["index"] = sim_idx |
For clarity on the files
rocketpy/simulation/monte_carlo.py
Outdated
outputs_dict = { | ||
export_item: getattr(monte_carlo_flight, export_item) | ||
for export_item in self.export_list | ||
} |
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.
outputs_dict = { | |
export_item: getattr(monte_carlo_flight, export_item) | |
for export_item in self.export_list | |
} | |
outputs_dict = { | |
export_item: getattr(monte_carlo_flight, export_item) | |
for export_item in self.export_list | |
} | |
outputs_dict["index"] = sim_idx |
Really useful to have index on both input and output
Converted to draft until you solve the remaining issues, specially the random number generation problem, |
53ba8ed
to
00d9d02
Compare
…s inside methods of Components
I believe this PR is ready again for another round of review. These are the changes since the previous review:
Overall, it seems that the time per iteration is even faster now, at least by my local measurements. @phmbressan might want to complement the information provided here, he knows this PR much better than I do! Please, make sure to take a careful look at the Monte Carlo .input file to check that there is indeed no dependency on the generated random variables. |
Another important issue: I currently can not interrupt the |
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.
This is a basic class, can we add unit tests to cover the modified lines?
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.
looks odd to have this file in the git tree... I think we should rebase the branch to develop
This pull request implements the option to run simulations in parallel to the
MonteCarlo
class. The feature is using a context manager namedMonteCarloManager
to centralize all workers and shared objects, ensuring proper termination of the sub-processes.A second feature is the possibility to export (close to) all simulation inputs and outputs to an .h5 file. The file can be visualized via HDF View (or similar) software. Since it's a not so conventional file, method to read and a structure to post-process multiple simulations was also added under
rocketpy/stochastic/post_processing
. There's a cache handling the data manipulation where a 3D numpy array is returned with all simulations, the shape corresponds to (simulation_index, time_index, column).column
is reserved for vector data, where x,y and z, for example, may be available under the same data. For example, undercache.read_inputs('motors/thrust_source')
time and thrust will be found.Pull request type
Checklist
black rocketpy/ tests/
) has passed locallypytest tests -m slow --runslow
) have passed locallyCHANGELOG.md
has been updated (if relevant)Current behavior
In the current moment, montecarlo simulations must run in parallel and all outputs a txt file
New behavior
The montecarlo simulations may now be executed in parallel and all outputs may be exported to a txt or an h5 file, saving some key data or everything.
Breaking change
Additional information
None