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

Morris cleanup #33

Merged
merged 14 commits into from
Jan 29, 2015
Merged

Morris cleanup #33

merged 14 commits into from
Jan 29, 2015

Conversation

jdherman
Copy link
Member

Hi @willu47 --

I took a shot at merging the different Morris methods into the main class. This will hopefully reduce confusion and eliminate some duplicate code. I think a lot of the Morris-related functions don't need to be in the main class, so I put them in a separate utility file and only brought in the most important group and optimal sampling functions. Do you think this makes sense?

I also made some small changes to read_param_file -- the detection of groups should be automatic now without needing to toggle it anywhere. Also made the "scaled" samples the default behavior since there was some issue about that in #31.

The test coverage is about the same as before. I'm stuck on how to test some of the sampling functions now that they're inside the Morris class. I added a test for #28 . Someday I'll get around to writing tests (at least regression ones) for the other methods. I also need to modify the other methods to use the Sample class.

Anyway, take a look when you have time and see what you think. This shouldn't break anything too much, it's more the organization I'm asking about. Thanks!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) when pulling 9febe11 on morris-cleanup into c1de4e4 on master.

…were commented out. morris sampling set up to accept 'problem' instead of parameter file, so user can optionally define in python
@jdherman
Copy link
Member Author

Added a little more to this, with two main changes:

  • Change the Morris sampling back from a class to just a set of functions. I think this makes the API easier to use, if we can just return numpy arrays instead of saving state behind the scenes. (for example Is there an API for Method of Morris? #35)
  • We can have the sampling methods accept a dictionary called problem instead of the parameter file name. problem can be created either by calling read_param_file(), or manually like this:
problem = {
   'num_vars': 3, 
   'names': ['x1', 'x2', 'x3'], 
   'groups': None, 
   'bounds': [[-3.14159265359, 3.14159265359], 
              [-3.14159265359, 3.14159265359], 
              [-3.14159265359, 3.14159265359]]
}

This is in response to a few requests to be able to create samples without a parameter file, which I think is a good idea. Right now only Morris is set up to do this, but I will update the other methods too.

I updated the tests to match, but I think something is weird with Travis+conda:

$ source activate testenv
We're making conda environments even better!  Part of this process
is changing the way environment activation works.  You need to
install the new conda-env package before you can use these feature:
    conda install -c conda conda-env
For more information, please visit: https://github.com/conda/conda-env
The command "source activate testenv" failed and exited with 1 during .

nosetests returns OK on my setup.

@willu47
Copy link
Member

willu47 commented Jan 29, 2015

Hi @jdherman. Sorry about the delay in replying - I've been distracted with other work recently. Your changes seem fair enough. I guess the partial move to OOP wasn't fully justified and unless a wholesale effort is made to refactor the entire codebase, it is much clearer to users to keep the functional programming style.

I'll have a look at the conda issues next week after a few deadlines have passed, and push to this branch (if that's okay).

Will

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.4%) to 38.44% when pulling a84b1ec on morris-cleanup into c1de4e4 on master.

@jdherman
Copy link
Member Author

Hi @willu47 -- no worries, I've been busy with other things here too. I think I solved the conda issue, it was just a matter of adding another line to install conda-env in travis.yml.

Yes, that's what I was thinking about OOP. It makes everything a more organized, but also a little harder to document and test (at least without a full refactor, like you said). Another issue is that since the methods can be used as a decoupled set of commands, it's not clear if the problem properties belong to the sampling method, the analysis method, or both. This "problem dictionary" idea is a sort of compromise.

Thanks for checking it out. I'll go ahead and merge after cleaning up a few things -- and then I'm going to finally get to writing some tests for the other methods besides morris.

@willu47
Copy link
Member

willu47 commented Jan 29, 2015

Ace!

First stop for tests could be to write high level regression tests so we can see whether the whole code package is working. For example, it might be nice to automate the use of the Sobol G-function and write tests that call Sobol-G with different numbers of variables and see whether they correctly estimate the sensitivity indices (within an acceptable degree of error).

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.33%) to 39.51% when pulling d4f2ce6 on morris-cleanup into c1de4e4 on master.

jdherman added a commit that referenced this pull request Jan 29, 2015
@jdherman jdherman merged commit b56d6eb into master Jan 29, 2015
@jdherman jdherman deleted the morris-cleanup branch January 29, 2015 16:16
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.

None yet

3 participants