Skip to content
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: Callback function for collecting additional data from Monte Carlo sims #702

Merged
merged 15 commits into from
Nov 22, 2024

Conversation

emtee14
Copy link

@emtee14 emtee14 commented Oct 6, 2024

Pull request type

  • Code changes (bugfix, features)

Checklist

  • Tests for the changes have been added (if needed)
  • Docs have been reviewed and added / updated
  • Lint (black rocketpy/ tests/) has passed locally
  • All tests (pytest tests -m slow --runslow) have passed locally
  • CHANGELOG.md has been updated (if relevant)

Current behaviour

Implementation for this feature suggestion #699 . At the moments users are limited to set variables when exporting data from Monte Carlo experiments.

New behaviour

Now a function can be passed to the MonteCarlo class which takes a Flight object as an argument and returns a dict to be appended to the default outputs.

Breaking change

  • No

Additional information

At the moment the implementation works but there is an existing issue with the RocketPyEncoder for writing the inputs to the .txt file as JSON strings. I've also made it check if the function overwrites existing variables though I feel this may not be necessary. I also had to fix how the previous results are imported as they are errors since text results can now be exported, see line 671.

@emtee14 emtee14 changed the base branch from master to develop October 6, 2024 21:18
@Gui-FernandesBR Gui-FernandesBR force-pushed the enh/monte-carlo-callback branch from 565cebd to a25c2ee Compare October 9, 2024 02:53
@Gui-FernandesBR
Copy link
Member

Thanks for opening the PR @emtee14 , we will get back to you after the EuRoC competition, when our development team will be more available.

Copy link

codecov bot commented Oct 9, 2024

Codecov Report

Attention: Patch coverage is 36.36364% with 14 lines in your changes missing coverage. Please review.

Project coverage is 75.99%. Comparing base (5856353) to head (e1a2061).

Files with missing lines Patch % Lines
rocketpy/simulation/monte_carlo.py 33.33% 12 Missing ⚠️
rocketpy/prints/monte_carlo_prints.py 50.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #702      +/-   ##
===========================================
- Coverage    76.08%   75.99%   -0.09%     
===========================================
  Files           95       95              
  Lines        10997    11015      +18     
===========================================
+ Hits          8367     8371       +4     
- Misses        2630     2644      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Lucas-Prates
Copy link
Contributor

@emtee14 thank you very much for opening this PR! I have been assigned to help you out with this feature. Your description in #699 and the (nice) implementation here give me a good idea of what you want to achieve.

To be more thorough, could you provide me an example of how you want to use it? Possibly a snippet of the code creating the export_function and calling the MonteCarlo class. It would be even better if it is an actual (maybe simplified?) example of what you are doing, e.g. exporting the environmental data mentioned in #699. If you want to give an example that is easy to run, consider the monte_carlo_class_usage.ipynb notebook and assume that this snippet would replace the code in the cell that calls the MonteCarlo class.

This would ensure that the design is best suited for the goal, and we could also add it as an example in the Monte Carlo usage notebook.

@emtee14
Copy link
Author

emtee14 commented Nov 11, 2024

Hi yes, I'll make an example that can be added to the docs. Another one of the uses we've found for it is exporting sensor data which we then use to run on our flight computer emulator allowing us to test firmware.

@Lucas-Prates Lucas-Prates marked this pull request as ready for review November 13, 2024 18:45
@Lucas-Prates Lucas-Prates requested a review from a team as a code owner November 13, 2024 18:45
@Lucas-Prates
Copy link
Contributor

Lucas-Prates commented Nov 13, 2024

This PR is ready for review. Along with the callback implementation provided by @emtee14, some error checks and tests were implemented. I provided a first simplified example on how to use this feature in the end of monte_carlo_class_usage.ipynb notebook, but am eager to see the example of @emtee14. Of course, his example can be provided in another PR as well, so there is no need to rush with the example.

Note: for some reason, codecov is not reflecting the test suite on the most recent commits. I am pretty sure I have covered most implemented lines.

Copy link
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you thought about the possibility of using both export_list and export_function at the same time?

@Lucas-Prates
Copy link
Contributor

Lucas-Prates commented Nov 15, 2024

have you thought about the possibility of using both export_list and export_function at the same time?

You mean using both in an example or replacing export_list functionality completely using export_function?

@Gui-FernandesBR
Copy link
Member

have you thought about the possibility of using both export_list and export_function at the same time?

You mean using both in an example or replacing export_list functionality completely using export_function?

have you thought about what would happen if someone uses both the export_list and export_function arguments at the same time (i.e. simultaneously)? Would that be a problem?

@Lucas-Prates
Copy link
Contributor

No problem at all! Indeed, the example I provided in the notebook uses both export_list and export_function simultaneously.

rocketpy/simulation/monte_carlo.py Outdated Show resolved Hide resolved
rocketpy/simulation/monte_carlo.py Outdated Show resolved Hide resolved
rocketpy/simulation/monte_carlo.py Outdated Show resolved Hide resolved
@Lucas-Prates
Copy link
Contributor

Sorry for the delay, but this PR is ready for review again. The following changes were made:

  1. The export_function argument had its name changed to data_collector;
  2. data_collector is a now a dict of python callables, reflecting the idea that we have several distinct callback functions instead of a "big" callback function as implemented previously;
  3. The check if computing the summary made sense by checking for a float was removed;
  4. The refactoring allowed for the validation of the data_collector to be done in the initialization of the MonteCarlo class;
  5. Docs and tests were updated accordingly.

@phmbressan
Copy link
Collaborator

Also, @Lucas-Prates could you add this PR to the CHANGELOG.md?

@Lucas-Prates Lucas-Prates merged commit ac4d3af into RocketPy-Team:develop Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

ENH: Callback function for collecting additional data from Monte Carlo sims
4 participants