Skip to content

Fix issue 43#44

Open
justushelo wants to merge 4 commits intoSimulation-Decomposition:mainfrom
justushelo:fix-issue-43
Open

Fix issue 43#44
justushelo wants to merge 4 commits intoSimulation-Decomposition:mainfrom
justushelo:fix-issue-43

Conversation

@justushelo
Copy link

Fixed sensitivity indices SOE calculation and additionally fix decomposition logic to follow Matlab.

Copy link
Member

@tupui tupui left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @justushelo !

Could you add a/some test which would have been failing without these changes and now pass?

inputs : DataFrame of shape (n_runs, n_factors)
Input variables.
output : DataFrame of shape (n_runs, 1)
output : DataFrame of shape (n_runs, 1) or (n_runs,)
Copy link
Member

Choose a reason for hiding this comment

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

Why doing the change? I personally find it easier to have a consistent shape.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I forgot to ask about that one. I added .flatten() to the parameter output = output.flatten() for both sensitivity_indices.py and decomposition.py as I think it can increase its robustness. Now when the function is called the output parameter can have both output = df['target'] and output = df[['target']]. Just forgot to ask that should it be changed in the docstring to include the "or (n_runs,)". Or if you find this change to do any good.

foe : ndarray of shape (n_factors, 1)
First-order effects (also called 'main' or 'individual').
soe : ndarray of shape (n_factors, 1)
soe_full : ndarray of shape (n_factors, 1)
Copy link
Member

Choose a reason for hiding this comment

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

Why renaming?

Copy link
Author

Choose a reason for hiding this comment

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

I thought it could help any future debugging to have the same variable name soe_full as it can be found in the Matlab function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants