-
-
Notifications
You must be signed in to change notification settings - Fork 136
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: Introducing local sensitivity analysis #575
base: develop
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #575 +/- ##
===========================================
- Coverage 73.69% 72.11% -1.59%
===========================================
Files 70 72 +2
Lines 10304 10550 +246
===========================================
+ Hits 7594 7608 +14
- Misses 2710 2942 +232 ☔ View full report in Codecov by Sentry. |
58b1cdf
to
1c7f84d
Compare
Tests are not passing
Could you fix it before our review, please? That would help us. @Lucas-Prates |
Sure, I will fix it briefly. This simplified type hinting started at python 3.9. I will make sure the tests pass this time. :P |
Please be aware of #444, we are not supporting type hinting or annotations yet. |
setup.py
Outdated
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.
Why adding a setup.py file?
We are using the pyproject.toml
file now. We no longer support the setup.py
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.
Indeed, I had this file from a previous pull request and thought the project still used setup.py
, sorry.
def set_parameters_nominal( | ||
self, | ||
parameters_nominal_mean, | ||
parameters_nominal_sd, | ||
): | ||
"""Set parameters nominal mean and standard deviation | ||
|
||
Parameters | ||
---------- | ||
parameters_nominal_mean : np.array | ||
An array contaning the nominal mean for parameters in the | ||
order specified in parameters names at initialization | ||
parameters_nominal_sd : np.array | ||
An array contaning the nominal standard deviation for | ||
parameters in the order specified in parameters names at | ||
initialization | ||
""" |
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.
So do you have to set the mean and sd simultaneously?
Also, do you have to set mean and sd of all parameters? Setting for just some of them does not work?
Another thing, to run a Monte Carlo sim, the mean and sd is already given in the Monte Carlo class right? So it would be natural to get them from there automatically
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.
It would be great to recover the mean and sd used in the simulation! I tried to find where it was stored, but I might have missed it. Do you know where it is stored?
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.
Indeed, you don't have to provide both. But I thought that, if you had one of them, you probably had the other.
if parameters_matrix.shape[1] != self.n_parameters: | ||
raise ValueError( | ||
"Number of columns (parameters) does not match number of parameters passed at initialization." | ||
) |
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.
Hasn't this already been checked if there is a parameters_matrix
?
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 are absolutely right, this check and the one in _estimate_target_nominal
are redundant.
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 just a partial review. I could not run your codes to validate it is working, but I suggested a few changes that might improve the quality (i.e. readability) of the code.
rocketpy/tools.py
Outdated
for i in range(n_parameters): | ||
parameter = parameters_list[i] | ||
parameters_matrix[:, i] = parameters_samples[parameter] | ||
|
||
for i in range(n_variables): | ||
target_variable = target_variables_list[i] | ||
target_variables_matrix[:, i] = target_variables_samples[target_variable] |
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.
Using enumerate is a more pythonic and readable solution.
for i in range(n_parameters): | |
parameter = parameters_list[i] | |
parameters_matrix[:, i] = parameters_samples[parameter] | |
for i in range(n_variables): | |
target_variable = target_variables_list[i] | |
target_variables_matrix[:, i] = target_variables_samples[target_variable] | |
for i, parameter in enumerate(parameters_list): | |
parameters_matrix[:, i] = parameters_samples[parameter] | |
for i, target_variable in enumerate(target_variables_list): | |
target_variables_matrix[:, i] = target_variables_samples[target_variable] | |
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.
Clear and useful, replaced most of the ranges
in code with enumerate
. Thanks!
rocketpy/tools.py
Outdated
# Auxiliary function that unnests dictionary | ||
def unnest_dict(x): | ||
new_dict = {} | ||
for key, value in x.items(): | ||
# the nested dictionary is inside a list | ||
if isinstance(x[key], list): | ||
# sometimes the object inside the list is another list | ||
# we must skip these cases | ||
if isinstance(value[0], dict): | ||
inner_dict = unnest_dict(value[0]) | ||
inner_dict = { | ||
key + "_" + inner_key: inner_value | ||
for inner_key, inner_value in inner_dict.items() | ||
} | ||
new_dict.update(inner_dict) | ||
else: | ||
new_dict.update({key: value}) | ||
|
||
return new_dict |
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 would define this function outside the load_monte_carlo_data()
function, this would allow us to re-use this function in other contexts.
Also, the term flatten
is usually used to describe this kid of "parse a nested dictionary" operation. Maybe it would be a good alternative of name.
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.
flatten_dict
is definitely a better name for it! With respect to defining it outside the load_monte_carlo_data
, there is a detail in the implementation that is specific to the way MonteCarlo
simulation data is saved. I have not defined it outside of the function in this first review round because of this, but I might think of a way to make it general and work with the MonteCarlo
data in the future.
…stall to setup.py
171ad30
to
8bbb7c4
Compare
Pull request type
Checklist
black rocketpy/ tests/
) has passed locallyCurrent behavior
The Sensitivity Analysis notebook teaches the users how to perform the simulations, plot the distribution
of some flight variables (e.g. apogee), and computes the prediction ellipses for the landing point.
New behavior
Our goal is to take sensitivity analysis even further. Briefly, we attempt to answer the following question: Which parameters would reduce the variability of the variable of interest (e.g. apogee) the most if we measured them with greater precision?
To that end, a bit of theory is developed, check the technical document. What was developed resembles the work of [1], a core reference in sensitivity analysis for engineering. His approach is a global sensitivity analysis with a full model containing interaction terms. Our first implementation considers a local sensitivity analysis using only first-order terms.
A quick and dirty test of the functionality of the SensitivityModel class is provided the "sensitivity_model_usage" notebook. This notebook is currently giving weird results! The linear approximations for the variables are, for some reason I still have to figure out, not good enough. This was not happening at previous experimentations that suggested that this approached worked. I have to look carefully at what is happening, but I did not want to delay the PR.
The concepts are discussed in-depth in the "sensitivity_analysis_parameter_importance" notebook (the notebook was not updated to the new SensitivityModel yet!)
Breaking change
Additional information
Technical Document
[1] Sobol, Ilya M. "Global sensitivity indices for nonlinear mathematical models and their Monte Carlo estimates." Mathematics and computers in simulation