Consider renaming fix_variables function to roof_duality

[arcondello]

Since we're releasing a new package, perhaps we should consider using a more explicit name. The deprecated dimod function can still be called fix_variables.

[hhtong]

I'm good with roof_duality() (would be clear to users that it does the same thing as RoofDualityComposite)

[JoelPasvolsky]

Given that it's a method of roof_duality module, is it worth considering a name that is clearer to users with less familiarity with roof duality? Perhaps something like reduce_variables()?

[randomir]

For the uninitiated, it's much more useful to have a function name that tells them what the function is doing, and not how.

I dare you all, try googling "roof duality" and quickly figuring out what a function in Ocean called roof_duality does.

[hhtong]

I'm wondering if reduce_variables() would be confusing though: I would think that reducing means that it would return the variables that still need to be solved for, but it returns what has already been fixed.

[arcondello]

Well, based on its location, I know it's a preprocessing algorithm for BQMs. There is also a docstring. And there are MANY ways to fix or reduce variables. And likely that we'll want to add some of them in the relatively near future.

I would rather err on the side of explicit.

determine_variable_assigments_with_roofduality works 😆

[JoelPasvolsky]

@arcondello, why would the func name need to specify that it's method is roof duality when method is imported from the roof duality module? You can have many fix_variables and reduce_variables methods that come from different modules and users will know which technique based on the module.

[randomir]

Exactly. Like fix_variables_roofduality, or dwave.preprocessing.fix_variables.roof_duality. Or dwave.preprocessing.roof_duality.fix_variables? 😆

[arcondello]

We could move the function into a more informative namespace?

from dwave.preprocessing.reductions import roof_duality`

[arcondello]

jinx.

[arcondello]

I prefer

library.catagory.algorithm

because it is most easily extensible. So

from dwave.preprocessing.reductions import roof_duality
from dwave.preprocessing.lower_bounds import roof_duality  # this is actually the most accurate but that's another story
from dwave.preprocessing.fix_variables import roof_duality
from dwave.preprocessing.partial_assignments import roof_duality

or something like that?

[amahmudwave]

My opinion

Reduction is too common of a term, reduce_variables is okay though.
partial_assignments --- Roof duality sometimes assigns all the variables.

I prefer the remaining two.

[arcondello]

If we merge this question with #2, then maybe it makes sense to move it into the lower_bounds space

from dwave.preprocessing.lower_bounds import roof_duality

lb, assignment = roof_duality(bqm)

make the lower_bound the more important value. Which is where roof duality actually comes from in the literature.

[arcondello]

Now, does the composite also live there?

# A
from dwave.preprocessing.lower_bounds import RoofDualityComposite
# B
from dwave.preprocessing.lower_bounds.roof_duality import RoofDualityComposite
# C
from dwave.preprocessing.composites import RoofDualityComposite

This is the kind of place that dwavesystems/dwave-ocean-sdk#21 would really pay off. Or a mono package 😄 .

[randomir]

I agree we should shift focus to lower_bounds, for the function.

But the composite is really about partially fixing variables, not finding lower bound, right? So, it probably warrants a clearer name.

That's even more so if we stick it in the dwave.composites.* flat namespace.

[arcondello]

Thinking a bit out of the box, what about

sampler = FixedVariableComposite(ExactSolver())

sampler.sample(bqm, fixed_variables= {'a': -1, 'b': 1})
sampler.sample(bqm, fixed_variables='roof_duality')

or something like that?

We could probably migrate the FixedVariableComposite from dimod while we're at it.

Alternative:

sampler = FixedVariableComposite(ExactSolver(), method='roof_duality')

sampler.sample(bqm, fixed_variables= {'a': -1, 'b': 1})
sampler.sample(bqm)

[arcondello]

I am using a string above because we don't have a stable API for these functions yet. It's not obvious to me that future ones would return lb, assignment in that order in that way.

[randomir]

Fixing variables is a natural fit to composite pattern, but looking on this as a black box ("I just want this thing to fix my variables"), I like this the most:

sampler = FixVariablesComposite(child_sampler, algorithm='roof_duality')

sampler.sample(bqm)

The one with fixed fixed variables:

sampler = FixVariablesComposite(child_sampler, algorithm='explicit', fixed_variables={'a': 1})

[arcondello]

I regret that we named the composite FixedVariablesComposite, we should rename it FixVariablesComposite (as you've done) and just make the old name an alias.

Should we leave it in dimod or move it here? If the latter, there are a bunch of other ones we could consider moving. SRTs, ConnectedComponents, etc.

[randomir]

I vote we move preprocessing stuff here. I can't see a particularly strong reason to keep it in dimod.

[JoelPasvolsky]

Do we classify SRT as pre-processing?

[arcondello]

@randomir I agree, especially in the long run. As a practical matter, some may be easier to move than others. Especially on a short timeline.

@JoelPasvolsky IMO it's no less a preprocessing algorithm than roof duality. Both have a "postprocessing" component.

Last chance to rename this package dwave.processing 😄

[randomir]

Btw, with this switching composite behavior (on algorithm/method), I'm thinking @singledispatchcomposite... 😆 (c.f. method version)

[hhtong]

Am also good with moving RoofDualityComposite into FixVariablesComposite.

To recap on naming and modules here, is everyone good with:

from dwave.preprocessing.lower_bounds import roof_duality

from dwave.preprocessing.composites import FixVariablesComposite, and any other preprocessing ones from dimod

[JoelPasvolsky]

From our discussion above, from dwave.preprocessing.lower_bounds import roof_duality might not be clear to many users that it fixes/reduces variables. So it highly favors an accurate naming versus a more accessible name. If we go with this we need to ensure that the surrounding documentation leads non-experts to this function.

[arcondello]

@JoelPasvolsky, I am not sure that I agree. We want non-experts to find the composite, which is pretty explicit. The function (for the users who want to use it to fix variables) is an implementation detail of the composite.

For the users who are interested in the lower bound, the function is in the appropriate namespace.

Of course both should be documented appropriately and given context. But this still seems like a massive step up from the current situation. Especially given how few users have found/used the existing function with the fix_variables name.

[hhtong]

Resolved with #7