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

BCG #69

Closed
wants to merge 29 commits into from
Closed

BCG #69

wants to merge 29 commits into from

Conversation

matbesancon
Copy link
Member

So BCG is "working" with quotes. On the example I tried it on in the tests, it finds a solution worse than what vanilla FW finds. I'll re-add logging to see if something comes up

@matbesancon
Copy link
Member Author

OK this is definitely broken at the moment. Primal values across iterations:
image

@matbesancon
Copy link
Member Author

Ok better now:
image

One problem still there is that if tolerance (epsilon) is increased, we end up back at the graph above. But with low tolerance we just oscillate at low values.

@matbesancon
Copy link
Member Author

Also, I used phi <= epsilon as termination condition, not dual_gap, which is only estimated at steps where we compute a new vertex.
Not sure this is in phase with the initial algorithm, which does not specify an early termination criterion

@matbesancon
Copy link
Member Author

OK this was a bug in the step back step from the simplex descent (when f(y) >= f(x)). Fixed now, converging nicely:
image

@matbesancon
Copy link
Member Author

BCG is now working. I will add more sophisticated logging with the number of steps of each type.

@pokutta
Copy link
Member

pokutta commented Jan 31, 2021

looks good - we should also "report" the step in the output, i.e., in the table -> there should be step types already: regular, dual and we might need to add simplex descent

@matbesancon
Copy link
Member Author

yes I wanted to get this done before merging this PR

@pokutta
Copy link
Member

pokutta commented Jan 31, 2021

perfect - can i run some tests etc?

@matbesancon
Copy link
Member Author

matbesancon commented Jan 31, 2021

yes definitely, the tests I ran for now are only partially covering the possible cases

@pokutta
Copy link
Member

pokutta commented Jan 31, 2021

ok will do today

@pokutta
Copy link
Member

pokutta commented Jan 31, 2021

i suppose bcg is also write out trajectory data for evaluation right?

@pokutta
Copy link
Member

pokutta commented Jan 31, 2021

Also, I used phi <= epsilon as termination condition, not dual_gap, which is only estimated at steps where we compute a new vertex.
Not sure this is in phase with the initial algorithm, which does not specify an early termination criterion

that's fine -> put you need to use 2phi <= epsilon i suppose as 2phi is the upper bound and phi is used for the step?

@pokutta
Copy link
Member

pokutta commented Jan 31, 2021

just pushed a test example where things repeatedly blew up and added a bit more extra reporting.

  • we also need to add back the "last" iterate reporting as in the other algorithms

@matbesancon
Copy link
Member Author

The logging has been updated to include number of non-simplex-GD steps. Things don't "blow up" anymore but the results seem strange (primal value way above the one that seems to appear for vanilla FW)

Comment on lines +185 to +190

if nforced_fw >= reset_threshold && forced_reset === false
active_set_cleanup!(active_set, weight_purge_threshold=100*weight_purge_threshold)
forced_reset = true
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added cleanup here -> works well in computations now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so cleanup works quite well now.

HOWEVER: two more problems

  1. now the algorithm behaves correctly and only executes SD steps once the active set is "final". does not make progress however. i suppose that this is a line search accuracy issue but could also be because the function value vs. gap spans 1e16 or so and it just might be a numerical issue
  2. more severely, if you run the "current" example, the algorithm throws tons of warnings. not sure why and where they come from. it is basically line 376+. i suppose this is because the SD should have executed the step and could be the same issue: too small for SD to make progress but then because one of these vertices still have enough weight large enough for afterwards?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in particular the primal values in this case are not monotonously decreasing -> something is wrong

@@ -355,7 +355,7 @@ function lp_separation_oracle(
kwargs...,
)
# if FW step forced, ignore active set
if !force_fw_step
if !force_fw_step && false # TODO: temporarily forced for demonstration
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so here i temporarily force the to run the lmo directly and something weird happens (already beforehand but now more pronounced: the steps are not convergent anymore -> run the example as is to see

@pokutta
Copy link
Member

pokutta commented Feb 2, 2021

Screen Shot 2021-02-03 at 00 33 22

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

2 participants