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.

Sign up for GitHub

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

Jump to bottom

Radial oat method #265

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

Radial oat method #265

wants to merge 43 commits into from
+1,578 −568

Conversation

ConnectedSystems
Copy link
Member

@ConnectedSystems ConnectedSystems commented Sep 19, 2019 •
edited
Loading

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
willu47
willu47 requested changes Sep 19, 2019
View reviewed changes
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.

src/SALib/sample/radial.py Outdated Show resolved Hide resolved
src/SALib/sample/radial.py Outdated Show resolved Hide resolved
src/SALib/sample/radial.py Outdated Show resolved Hide resolved
src/SALib/analyze/radial_st.py Outdated Show resolved Hide resolved
@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 •
edited
Loading

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

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

@ConnectedSystems ConnectedSystems linked an issue Apr 21, 2020 that may be closed by this pull request
Add new Method - Radial sampling strategy for Method of Morris #132
Open
@ConnectedSystems ConnectedSystems mentioned this pull request Jun 17, 2020
Closed
@lbteixeira
Copy link
Contributor

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
Merged
@willu47 willu47 moved this from v1.5 onward to v1.4 in SALib Development Roadmap Jul 6, 2020
@ConnectedSystems
Actually handle Zero division
da99fda 
force return value again
@ConnectedSystems ConnectedSystems moved this from v1.4 to v1.5 onward in SALib Development Roadmap Jun 27, 2021
@ConnectedSystems ConnectedSystems moved this from v1.5 onward to 1.4.x series in SALib Development Roadmap Jun 27, 2021
@ConnectedSystems ConnectedSystems moved this from 1.4.x series to v1.5 onward in SALib Development Roadmap Sep 21, 2021
@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
Reviewers

@willu47 willu47 willu47 approved these changes

Assignees
No one assigned
Labels
add_method
Projects
SALib Development Roadmap
v1.5 onward
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add new Method - Radial sampling strategy for Method of Morris
3 participants
@ConnectedSystems @lbteixeira @willu47