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

Finding local maximum distance (after Ruano et al. 2012) as an alternative to the brute force approach. #82

Merged
merged 8 commits into from Nov 26, 2015

Conversation

JoerivanEngelen
Copy link
Contributor

I added a way to find the local maximum distance after Ruano et al. 2012, as alternative to the brute force approach of Campolongo et al. It speeds up the process for especially more trajectories, which can be read in their paper:

"An improved sampling strategy based on trajectory design for application of the Morris method to systems with many input factors". (2012) Environmental Modelling & Software 37

It is now possible to optimize for more than 8 trajectories, without the use of commercial software (Gurobi). The method does not necessarily always find the largest global distance between trajectories, but it still does a good job and is preferable over not doing any optimization.

I am looking forward to your feedback on my coding.

…the brute force approach. I should still write some tests for it.
…example, it should match the global maximum distance.
@jdherman
Copy link
Member

jdherman commented Nov 5, 2015

Thanks Joeri! I'll defer to @willu47 to check this out, since the Morris optimization is his area.

The Travis build failed for Python 3, but it looks like this might have been a setup issue not related to your pull request. We'll have to investigate.

@willu47
Copy link
Member

willu47 commented Nov 6, 2015

Great! Thanks so much @JoerivanEngelen. I will have a play with your code and see if I have any suggestions.

@willu47
Copy link
Member

willu47 commented Nov 9, 2015

Hi @JoerivanEngelen. I've added a few suggestions to your code. The procedure seems to work fine, except in the following situations:

  • For N greater than 500 or so (see comment in code)
  • If you already have gurobi installed (like me), there's no way to prevent the global optimisation from running in favour of the local procedure that you've coded up...
  • There's only one test ;o). Ideally you should test each of the procedures you have written. 👍 👍
    We'll keep the pull request open and you can keep pushing any changes you want to make. Just let us know when you are done.

Great work on this! Its good to see others getting into the code and further developing the library.

@jdherman That Python 3 bug has got me puzzled. It fails on my laptop too when run in Python 3. I suspect it is something to do with the latest version of python 3 and setuptools/disttools. I can't find much online about it though :o(

@JoerivanEngelen
Copy link
Contributor Author

Thanks for the good feedback! I was slacking a lot on the testing; will update that. I'm not sure how the test procedure works though: Does it test the package each time you run a function from the package or only when the user explicitly commands "test package"? When I find the time (and I am currently unemployed, so that will not last long ;-) ), I will make changes and push them!

@willu47
Copy link
Member

willu47 commented Nov 10, 2015

Hi @JoerivanEngelen. The tests can be run in two ways. You can navigate to the SALib folder and run nosetests, or you can run the command python setup.py test. Both run nose, which then looks for all the tests in the tests folder (anything with test_ in the file, module or procedure name) and runs them.

Test driven development is a good principle by which to work. You define your tests first and write a procedure or module to pass the test (and no more). This allows you to develop software quickly and efficiently, and can help avoid feature bloat - where you keep adding features (because you can). The other advantage of unit testing is that you can be fairly sure that if each of your modules of code work as you expect (as defined by the tests), then your programme will also work as expected.

The tests in the existing test modules, such as test_morris.py follow obvious naming conventions, as you've found out. Often, the first thing to do following the discovery of a bug is to write a test which recreates the bug. Then, running just that test, work out what's going wrong and correct the code until the test passes. Then run all the tests to check your changes didn't affect the other tests. Always run the tests before you commit your changes to the repository or submit a pull request.

Check out the software carpentry website for more really helpful hints on testing and more (basically almost everything I know about programming came from this website and courses at my uni).

@JoerivanEngelen
Copy link
Contributor Author

Thanks for the tips/explanations! I was aware of test driven development, but for some reason (lazyness probably) I have never used it. Will work on that

@JoerivanEngelen
Copy link
Contributor Author

I am currently updating my pull request, does it automatically update when I push my local git folder to Github? Or do I have to pull a new request and close this one?

@willu47
Copy link
Member

willu47 commented Nov 24, 2015

You can just push your repository and it will automatically update.

… ==. Furthermore I included extra tests for each function and the calculation in the sum_distances function is now vectorized
@JoerivanEngelen
Copy link
Contributor Author

Updated it, but the Travis CI checks now both failed. For Python 2.7, for some reason it cannot import the module 'tests'. As far as I know, the Travis build failed for things I did not mess around with in terms of scripting. However, in Git I staged all files (I think also the ones that are ignored in the "ignore file"), could this be the reason for this test failure?

@willu47
Copy link
Member

willu47 commented Nov 26, 2015

Hi @JoerivanEngelen. Thanks for your updates. I managed to recreate the error in Python 2.7 and 3.3. The former is due to a change in the matplotlib codebase in v1.5.0. I'm looking into it. I'm still not sure what the problem is for Python 3.x

Update: Fixed in the latest version, but not yet on conda (which we use on Travis)

@willu47
Copy link
Member

willu47 commented Nov 26, 2015

This describes the error we're getting with Python 3.x - it's to do with a latest version of setuptools v18.4 (also this).

@JoerivanEngelen
Copy link
Contributor Author

Good find! How should we go further right now: will you look into these errors or do we split tasks? As in: Is there anything on this I can/should do?

@willu47
Copy link
Member

willu47 commented Nov 26, 2015

@JoerivanEngelen , I've made a pull request for you to merge into your code. That should fix the setuptools bug. The matplotlib bug should fix itself as and when the next version of matplotlib is released. If you merge my (small) change on your branch we can proceed with merging your pull-request.

@willu47
Copy link
Member

willu47 commented Nov 26, 2015

BTW - the tests look great. When the tests pass, we should see a nice little stats increase in Coveralls.

willu47 and others added 2 commits November 26, 2015 14:49
Removed TestCommand setup due to setuptools update
@willu47
Copy link
Member

willu47 commented Nov 26, 2015

Hmm. Okay, that's wasn't as successful as I hoped!

@willu47 willu47 closed this Nov 26, 2015
@willu47 willu47 reopened this Nov 26, 2015
willu47 added a commit that referenced this pull request Nov 26, 2015
Finding local maximum distance (after Ruano et al. 2012) as an alternative to the brute force approach.
@willu47 willu47 merged commit 2513de0 into SALib:master Nov 26, 2015
@JoerivanEngelen
Copy link
Contributor Author

I assume it works now? :)

@willu47
Copy link
Member

willu47 commented Nov 27, 2015

Yep, all tests passed. For some reason, the merge of my commits into your branch confused Travis CI (which runs the tests) and it was running with code from the previous commits.

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