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

Radial oat method #265

Open
wants to merge 43 commits into
base: main
Choose a base branch
from

Conversation

ConnectedSystems
Copy link
Member

@ConnectedSystems ConnectedSystems commented Sep 19, 2019

Address #132

Radial OAT sampling and analysis method.
Includes Elementary Effects and Jansen's sensitivity estimation.

Still a work in progress and not ready for merge.

Requires:

  • Additional testing and code review (by someone other than me)
  • Clean up/additional docstrings
  • CLI interface functions
  • Tests for CI
  • Example scripts

@willu47 willu47 self-requested a review September 19, 2019 13:03
Copy link
Member

@willu47 willu47 left a comment

Choose a reason for hiding this comment

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

Hi @ConnectedSystems. Many thanks for this pull request which tackles issue #132

I've made some comments on syntax inline.

Is it worth adding this as a strategy within the Morris submodule? And do the analyze methods differ significantly from the existing implementations?

As you state, there's still a fair amount of work on implementing tests for these new modules. It may be worth parameterising existing tests for the morris method with the different methods. Ideally, they should all produce the same values.

Happy to iterate on this.

@ConnectedSystems
Copy link
Member Author

I've made some comments on syntax inline.

Thanks - for some I'm obviously reliant on a linter which usually handles styling for me - it appears not to be working for me at the moment but have addressed these manually.

Is it worth adding this as a strategy within the Morris submodule?

I was going to ask about this. Wasn't sure how best cleanly incorporate it at first glance but will take a more in-depth look again tomorrow.

And do the analyze methods differ significantly from the existing implementations?

I think they differ enough to warrant separate implementations but happy to be corrected on this. I plan to have a closer examination when I have more time later on.

As you state, there's still a fair amount of work on implementing tests for these new modules. It may be worth parameterising existing tests for the morris method with the different methods. Ideally, they should all produce the same values.

The same values, as in, the results between Morris and Radial OAT should be identical?
From my initial testing, this is not the case but the rankings were the same.

@ConnectedSystems
Copy link
Member Author

ConnectedSystems commented Oct 7, 2019

Still todo:

  • Tests for CI
    • sequential use works as intended
    • sobol_jansen and sobol results should match
    • rank order of radial_ee and morris should at least match (I believe) if not the mu_star values
  • CLI interface functions
  • Example scripts

@ConnectedSystems
Copy link
Member Author

ConnectedSystems commented Nov 25, 2019

Excuse the delay on this - been busy with a truckload of other things.

Latest round of commits include an initial support for the CLI and changes to the docstrings to better address last review comments.

Preliminary tests produces results in line with what was reported in Campolongo et al. (2011), although I don't get the exact quoted index values - probably because I used a much lower sample size (10,000 - 50,000 compared to the reported 172,032 in the paper).
EDIT: Misread what the paper was saying here. Additional confusion came from the reported analytic values which appear to be incorrect.

Specifically, using the modified G-function (with 20 parameters) with the parameter values specified in Table 3 of the cited article above:

Identified parameters of importance for G_4: 2, 8, 11, 18

G4_ee_results
G4_st_results

Identified parameters of importance for G_10: 2, 7, 8, 9, 10, 11, 12, 15, 18, 20

G10_ee_results

G10_st_results

@ConnectedSystems
Copy link
Member Author

Almost there - biggest job now is to add support for usage via CLI

@ConnectedSystems
Copy link
Member Author

Finalized tests and initial CLI support.

Still to do:

  • Finalize example scripts for documentation, which will lead to final CLI support
  • Clean up code where needed

@ConnectedSystems
Copy link
Member Author

This PR ready for review

@lbteixeira
Copy link

Hi, @ConnectedSystems.

In #320 you mentioned this topic, and I could help if you need. Do you have a more or less working code? Or perhaps I could start from scratch?

@ConnectedSystems
Copy link
Member Author

Hi @lbteixeira

This specific implementation is working. If you care to take a look you can check out the branch in my repository.
The branch name is radial-oat-method (https://github.com/ConnectedSystems/SALib/tree/radial-oat-method).

What would be nice is to make this compatible with the Morris strategy class

I have not looked into this in detail so I am not sure if the data structures are compatible and if not, what would be required to make them compatible.

While it would be nice to merge things, I also don't want you to spend too much time on this.

@ConnectedSystems ConnectedSystems mentioned this pull request Jul 5, 2020
force return value again
@willu47 willu47 changed the base branch from master to main July 12, 2022 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add new Method - Radial sampling strategy for Method of Morris
3 participants