lazied_dicg #532
lazied_dicg #532
Conversation
WenjieXiao-2022
force-pushed
the
dicg
branch
from
58608d4 to
b99a468
Compare
src/dicg.jl
Outdated
| extra_vertex_storage=nothing, | ||
| lazy_tolerance=2.0, | ||
| memory_mode::MemoryEmphasis=InplaceEmphasis(), | ||
| variant = "standard", |
There was a problem hiding this comment.
Since you only have two variants it might be cleaner to use a boolean flag here. For example, using_blended which would be true if blended_dicg was used and false otherwise.
src/dicg.jl
Outdated
| memory_mode::MemoryEmphasis=InplaceEmphasis(), | ||
| variant = "standard", | ||
| strong_lazification = false, | ||
| use_extra_vertex_storage = false, |
There was a problem hiding this comment.
If this is a specific function for the lazy version of dicg, we always use the extra vertex storage, no? In which case, the default value for use_extra_vertex_storage should be true.
src/dicg.jl
Outdated
| if !strong_lazification | ||
| a = a_taken | ||
| d = muladd_memory_mode(memory_mode, d, a, v_taken) | ||
| step_type = ST_PAIRWISE | ||
| away_index = -1 | ||
| else | ||
| a = a_taken | ||
| d = muladd_memory_mode(memory_mode, d, a, v_taken) | ||
| step_type = ST_PAIRWISE | ||
| away_index = a_local_loc | ||
| end |
There was a problem hiding this comment.
The only thing that is different is away_index, isn't it? That can be done cleaner as
away_index = strong_lazification ? a_local_loc : -1
src/dicg.jl
Outdated
| away_index = a_local_loc | ||
| end | ||
| else | ||
| a = compute_inface_extreme_point(lmo, NegatingArray(gradient), x) |
There was a problem hiding this comment.
You seem to be calling this twice! For Blended in line 559 and for non strong lazification in line 538.
| return reshape(a, n, n) | ||
| end | ||
|
|
||
| # Fast way to compute gamma_max. | ||
| # Check every constraint and compute the corresponding gamma_upper_bound. | ||
| function dicg_maximum_step(lmo::MathOptLMO{OT}, direction, x) where {OT} | ||
| function dicg_maximum_step(lmo::MathOptLMO{OT}, direction, x;tol=1e-6) where {OT} |
There was a problem hiding this comment.
It would be good to have a doc string here, i.e.
"""
Description of the function.
"""
This would also be good for the compute_inface_extreme_point as well as is_inface_feasible function.
|
|
||
| if grad_dot_a_taken - grad_dot_v_inface >= phi && | ||
| grad_dot_a_taken - grad_dot_v_inface >= epsilon | ||
| step_type = ST_LAZY |
There was a problem hiding this comment.
How is your convention here? Is a step lazy as soon as one of the vertices was computed lazily?
There was a problem hiding this comment.
Yes, as long as one of vertices is lazifized, it is called lazy here
No description provided.