Closed wanderer28 closed 6 years ago
wanderer28
commented
6 years ago 
gnannicini
commented
6 years ago Could you limit the modifications to what is necessary in the code base? In particular, let us skip Mopossum.py for now, but we can try to support the modifications in the code base that you need for Mopossum to work as a stand-alone project. (We can integrate it later if necessary, but it will require more testing.)
Also, please explain to me the logic of the changes: I see some new limits on number of cycles, please tell me how they work and ensure they would not impact current users.
Thanks!
Tomalwo
commented
6 years ago Hi Giacomo,
Apologies, the latest push that included Mopossum wasn't intended for the merge. So yes, it's not necessary to include it.
The idea for Mopossum is to run for x number of cycles, then reweigh the objective sum, and to restart (using the points from the previous cycles instead of sampling).
The limit on the number of cycles is intended as an alternative to max_evals and max_iterations (in line 839 of rbfopt_algorithm). The check for this is not at the start of the loop to ensure that we have finished the current cycle. I don't think this would impact other users, beyond the default for this new setting. (Currently it's 50, but it could be any number.) This is implemented only for serial optimization.
We also added thresh_cycle_unlimited_refinement, as an alternative to thresh_unlimited_refinement (in line 190 of rbfopt_algorithm). This threshold considers the number of stalled iterations relative to maximum number that will trigger a restart. The idea is to ensure that we can benefit from refinement before a restart, and thus perhaps avoid it. (Assuming that the restart would be triggered by stalling.) I just realized that this setting is present in the settings file, but not documented. This change is unrelated to MOpossum and intended to improve RBFOpt in terms of local exploitation, but I haven't gotten around to testing this yet.
In line 232 of rbfopt_algorithm, we have added an optional argument on whether to execute the sampling phase or not. This is to avoid sampling when we keep restarting RBFOpt for Mopossum, since we are adding points and values from previous runs anyway. But the default in the current version on GitHub is False, which is a mistake. It should be True instead. I've also found that some additional changes are required to make this work, which are not on GitHub yet.
These changes involve ignoring the distance check when there's no sampling, which perhaps is not ideal.
I'm thinking of performing an alternative distance check on the points that we're adding instead.
On reflection, I think that we should clean up the issues I mentioned first and that the merge request was premature. Will you have some these days if I upload a revised version?
The good news is that I have a raw version of MOpossum working. I'm still figuring out how to test, i.e. benchmark it, but the general idea seems to work. (Currently we're using a Sobol series for the weights, which isn't great, since the values don't always add up to one.)
I hope this helps, and let me know if you have any comments!
Cheers, Thomas
On Thu, Aug 9, 2018 at 4:46 AM, Giacomo Nannicini notifications@github.com wrote:
Could you limit the modifications to what is necessary in the code base? In particular, let us skip Mopossum.py for now, but we can try to support the modifications in the code base that you need for Mopossum to work as a stand-alone project. (We can integrate it later if necessary, but it will require more testing.)
Also, please explain to me the logic of the changes: I see some new limits on number of cycles, please tell me how they work and ensure they would not impact current users.
Thanks!
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/coin-or/rbfopt/pull/13#issuecomment-411546558, or mute the thread https://github.com/notifications/unsubscribe-auth/APHirqH5k12U0nJP5QPUILT1SdFotvy5ks5uO04tgaJpZM4UfoQA .
gnannicini
commented
6 years ago Sounds good. Yes, please work on these issues. I will close the PR for now.
Tomalwo
commented
6 years ago Hi Giacomo,
I have fixed the issues I have pointed out in my last email and created a new pull request. The RBFOpt version on GitHub has some differences to mine due to your last update, I hope that won't create too many issues!
Let me know when you've had the chance to have a look!
Cheers, Thomas
On Thu, Aug 9, 2018 at 9:37 AM, Thomas W thomas.wortmann@gmail.com wrote:
Hi Giacomo,
Apologies, the latest push that included Mopossum wasn't intended for the merge. So yes, it's not necessary to include it.
The idea for Mopossum is to run for x number of cycles, then reweigh the objective sum, and to restart (using the points from the previous cycles instead of sampling).
The limit on the number of cycles is intended as an alternative to max_evals and max_iterations (in line 839 of rbfopt_algorithm). The check for this is not at the start of the loop to ensure that we have finished the current cycle. I don't think this would impact other users, beyond the default for this new setting. (Currently it's 50, but it could be any number.) This is implemented only for serial optimization.
We also added thresh_cycle_unlimited_refinement, as an alternative to thresh_unlimited_refinement (in line 190 of rbfopt_algorithm). This threshold considers the number of stalled iterations relative to maximum number that will trigger a restart. The idea is to ensure that we can benefit from refinement before a restart, and thus perhaps avoid it. (Assuming that the restart would be triggered by stalling.) I just realized that this setting is present in the settings file, but not documented. This change is unrelated to MOpossum and intended to improve RBFOpt in terms of local exploitation, but I haven't gotten around to testing this yet.
In line 232 of rbfopt_algorithm, we have added an optional argument on whether to execute the sampling phase or not. This is to avoid sampling when we keep restarting RBFOpt for Mopossum, since we are adding points and values from previous runs anyway. But the default in the current version on GitHub is False, which is a mistake. It should be True instead. I've also found that some additional changes are required to make this work, which are not on GitHub yet.
These changes involve ignoring the distance check when there's no sampling, which perhaps is not ideal.
I'm thinking of performing an alternative distance check on the points that we're adding instead.
On reflection, I think that we should clean up the issues I mentioned first and that the merge request was premature. Will you have some these days if I upload a revised version?
The good news is that I have a raw version of MOpossum working. I'm still figuring out how to test, i.e. benchmark it, but the general idea seems to work. (Currently we're using a Sobol series for the weights, which isn't great, since the values don't always add up to one.)
I hope this helps, and let me know if you have any comments!
Cheers, Thomas
On Thu, Aug 9, 2018 at 4:46 AM, Giacomo Nannicini < notifications@github.com> wrote:
Could you limit the modifications to what is necessary in the code base? In particular, let us skip Mopossum.py for now, but we can try to support the modifications in the code base that you need for Mopossum to work as a stand-alone project. (We can integrate it later if necessary, but it will require more testing.)
Also, please explain to me the logic of the changes: I see some new limits on number of cycles, please tell me how they work and ensure they would not impact current users.
Thanks!
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/coin-or/rbfopt/pull/13#issuecomment-411546558, or mute the thread https://github.com/notifications/unsubscribe-auth/APHirqH5k12U0nJP5QPUILT1SdFotvy5ks5uO04tgaJpZM4UfoQA .