[WIP] Add samplers submodule #58
Conversation
kevinchern
left a comment
kevinchern
left a comment
There was a problem hiding this comment.
Thanks Theo, this is looking good and motivates some topics for discussion. A purpose of the changes proposed in this PR is to decouple models and samplers. In essence, my comments are concerned with this goal. For example, objects like GRBMs should no longer have a to_ising method, as that method is specific to dimod models and dimod sampling. As a consequence (maybe tangentially), I propose we wrap and hide dimod and dimod-motivated objects (e.g., sample sets, and to_ising).
| """PyTorch plugin wrapper for a dimod sampler. | ||
|
|
||
| Args: | ||
| module (GraphRestrictedBoltzmannMachine): GraphRestrictedBoltzmannMachine module. Requires the |
There was a problem hiding this comment.
Any particular reason to name it module instead of grbm?
There was a problem hiding this comment.
Not really. Remnant from wanting it to be a generic module but not having a reasonable base class to use.
|
|
||
| Args: | ||
| module (GraphRestrictedBoltzmannMachine): GraphRestrictedBoltzmannMachine module. Requires the | ||
| methods ``to_ising`` and ``nodes``. |
There was a problem hiding this comment.
With this new interface, to_ising method should be removed from GRBM and implemented elsewhere as a function (not in this sampler either).
| self, | ||
| module: GraphRestrictedBoltzmannMachine, | ||
| sampler: dimod.Sampler, | ||
| prefactor: float, |
There was a problem hiding this comment.
Should we remove prefactor, linear_range, and quadratic_range from DimodSampler?
This restriction is primarily motivated by a QPU's limitations and not a typical constraint in classical samplers.
I need to give this more thought..
| return sampleset_to_tensor(self._module.nodes, self._sample_set, self.device) | ||
|
|
||
| @property | ||
| def sample_set(self) -> dimod.SampleSet: |
There was a problem hiding this comment.
Remove property and keep the sample set a hidden attribute; the public interface should not be concerned with dimod objects.
There was a problem hiding this comment.
This was requested by @VolodyaCO for the GRBM class previously, so it seems useful, no?
thisac
force-pushed
the
feature/samplers
branch
2 times, most recently
from
3c93210 to
60c3cee
Compare
thisac
force-pushed
the
feature/samplers
branch
from
60c3cee to
2e55e9c
Compare
2 participants
Draft of a samplers submodule. Still missing full test coverage, and a consensus on whether this is what we want or not.