The Wayback Machine - https://web.archive.org/web/20200910202507/https://github.com/GPflow/GPflowOpt/pull/107
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

Bug fix in pareto.py, Pareto::divide_conquer_nd #107

Merged
merged 3 commits into from Sep 12, 2018
Merged

Conversation

@smanist
Copy link
Contributor

smanist commented Sep 6, 2018

The algorithm in Pareto::divide_conquer_nd fails when two points in the Pareto set have the same value in a certain dimension. An example is included in the modified pareto.py:
The Pareto set d21 contains three points, two of which have a value of 2.0 in the first dimension. Ordering the three points results in different ways results in different values of the hypervolume (28 and 32), which are all wrong (should be 29).

The issue is in the dominance test associated with _is_test_required method and pseudo_pf. The array pseudo_pf assigns different ranks to the same values. Therefore, by reordering the Pareto set in the test case, different pseudo Pareto sets are generated for the same Pareto set.

I figured out two ways for the fix. One is to fix pseudo_pf by sorting the Pareto set such that the same values are assigned the same rank (e.g. using scipy.stats.rankdata). The other is to fix the dominance test by checking the actual Pareto set. The first one leads to more iterations in the algorithm. Therefore, I implemented the second approach in pareto.py with a minimum modification - although some simplifications of the code are possible.

Copy link
Contributor

nknudde left a comment

Thank you for this bugfix. Could you please remove the non-necessary lines (208, 214, 283-297)? Then we can proceed and merge your fix with master.

@icouckuy
Copy link
Contributor

icouckuy commented Sep 7, 2018

Thanks. Maybe you can turn that code into a test in
https://github.com/GPflow/GPflowOpt/blob/master/testing/unit/test_pareto.py

Either adapt test_hypervolume to include this particular case, or make a second test_hypervolume.

@codecov-io
Copy link

codecov-io commented Sep 7, 2018

Codecov Report

Merging #107 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #107      +/-   ##
==========================================
+ Coverage   99.91%   99.91%   +<.01%     
==========================================
  Files          18       18              
  Lines        1118     1121       +3     
==========================================
+ Hits         1117     1120       +3     
  Misses          1        1
Impacted Files Coverage Δ
gpflowopt/pareto.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c03e23...79cffab. Read the comment docs.

@smanist
Copy link
Contributor Author

smanist commented Sep 7, 2018

Hi, I have removed unnecessary lines as @nknudde requested and added a unit test case as @icouckuy suggested. However, I did not find the script/interface for running/checking the unit test case. I hope it works.

@icouckuy
Copy link
Contributor

icouckuy commented Sep 12, 2018

@smanist can you update your master to the last one. Hopefully this will also trigger all tests so I can merge this (in the future it is perhaps better to have a separate branch for a PR instead of master). Thanks.

@smanist
Copy link
Contributor Author

smanist commented Sep 12, 2018

Hi @icouckuy, github shows that my fork is three commits ahead of the master. Anyway, it looks like by now the code has passed the Travis CI check and is waiting for the coverage check results.
Also, how will the change request be resolved? Automatically by github or manually by @nknudde?

@icouckuy icouckuy merged commit f1c268e into GPflow:master Sep 12, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.