ENH: Callback function for collecting additional data from Monte Carlo sims #702
Conversation
…xisting functions for compatability
Gui-FernandesBR
force-pushed
the
enh/monte-carlo-callback
branch
from
565cebd
to
a25c2ee
Compare
|
Thanks for opening the PR @emtee14 , we will get back to you after the EuRoC competition, when our development team will be more available. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #702 +/- ##
===========================================
+ Coverage 75.95% 75.99% +0.04%
===========================================
Files 99 95 -4
Lines 11237 11015 -222
===========================================
- Hits 8535 8371 -164
+ Misses 2702 2644 -58 ☔ View full report in Codecov by Sentry. |
|
@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 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. |
|
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. |
…ates/impact_state)
… any kind (attribute, logical, etc)
|
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. |
| if key in self.export_list: | ||
| raise ValueError( | ||
| f"Invalid export function, returns dict which overwrites key, '{key}'" | ||
| ) |
There was a problem hiding this comment.
We only need to do this once, I recommend moving this validation to somewhere in the init method so we don't need to re-run it every time.
There was a problem hiding this comment.
It is a good idea to run this only once, but moving it to the init method might be problematic in the current implementation. The reasoning is as follows: to validate that there are no keys being overwritten, we have to call export_list so we can have the dictionary outputted by the callback. However, in order to make that call, we have to have the data from the simulations. I see two options:
- Create a "mock dictionary", or at least its keys, from
export_functionand make the test in the init method; - Create a flag to check if
export_functionwas validated. On the first call of these lines (inside__export_flight_data, we make the test and set the flag, hence halting further validations.
There was a problem hiding this comment.
It is a good idea to run this only once, but moving it to the init method might be problematic in the current implementation. The reasoning is as follows: to validate that there are no keys being overwritten, we have to call
export_listso we can have the dictionary outputted by the callback. However, in order to make that call, we have to have the data from the simulations. I see two options:
- Create a "mock dictionary", or at least its keys, from
export_functionand make the test in the init method;- Create a flag to check if
export_functionwas validated. On the first call of these lines (inside__export_flight_data, we make the test and set the flag, hence halting further validations.
@Lucas-Prates if we have a __check_export_list method, I think we should also have a __check_export_function as well. I like the idea of a mock_flight, but I don't know how hard it would be to make it happen.
I believe my other comment is related to this. If instead of a "callback function" that evaluates and returns a dictionary, maybe we should receive a dictionary with different callback functions. That way we could easily get the expected key names.
Other option is to not make any validation, let the user deal with the fact they will be overwriting the values if use the same name of variables in the dictionary.
| export_function : callable, optional | ||
| A function which gets called at the end of a simulation to collect | ||
| additional data to be exported that isn't pre-defined. Takes the | ||
| Flight object as an argument and returns a dictionary. Default is None. |
There was a problem hiding this comment.
I recommend specifying the dictionary structure. What are the keys and values? Can you give an example to anticipate users' curiosity?
Other thing is that once I read the name export_function I automatically imagine a callback function that returns a float, for example:
def mach_number(flight: Flight) -> float:
return ...
export_function= mach_numberTherefore, unless this is only me that has such impression, I believe we could propose a better name for the argument. Maybe data_collectoror additional_data_collector.
You mean using both in an example or replacing |
have you thought about what would happen if someone uses both the |
Pull request type
Checklist
black rocketpy/ tests/) has passed locallypytest tests -m slow --runslow) have passed locallyCHANGELOG.mdhas 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
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.