Conversation
Qommunity/iterative_searcher/iterative_hierarchical_searcher.py
Outdated
Show resolved
Hide resolved
| # List instead of samplesets_data = np.empty((num_runs), dtype=object) | ||
| # To prevent jupyter notebook kernel crashes | ||
| # as handling big objects is not efficient with numpy dtype=object arrs | ||
| samplesets_data = [] |
There was a problem hiding this comment.
Maybe, it may be expanded later with embeddings returning/nicer results interface, so you kind of sensed it right that it might be changed. But a list object for now. Is it ok? I was rather explaining why a list not np.empty like the rest.
| chain_strength: float | None = None, | ||
| use_clique_embedding: bool = False, | ||
| elapse_times: bool = False, | ||
| return_sampleset_metadata: bool = True, |
There was a problem hiding this comment.
Rename the variable accordingly to what you did in QHyper. As I suggested earlier, this flag should be in the method, not in the constructor
There was a problem hiding this comment.
Yes, but it's connected to the issue I opened qc-lab/QHyper#57
and to the QHyper.Advantage.init method. Please take a look at the analogical comment in QHyper's PR:
qc-lab/QHyper#55 (comment).
| self.use_clique_embedding = use_clique_embedding | ||
| self._use_weights = use_weights | ||
| self.elapse_times = elapse_times | ||
| self.return_sampleset_metadata = return_sampleset_metadata |
There was a problem hiding this comment.
rename the variable name accordingly to what you did in QHyper
There was a problem hiding this comment.
Done. May I ask what is a good practice, shall I make them private (with underscore _)? HierarchicalSearcher accesses it (self.sampler.return_metadata).
|
|
||
| def sample_qubo_to_dict(self) -> dict: | ||
| sample = self.advantage.solve() | ||
| def sample_qubo_to_dict(self, return_sampleset_metadata: bool | None = None) -> dict: |
There was a problem hiding this comment.
There is no reason to use two flags for the same thing: one as a method argument, second as a constructor argument. Leave this one and remove one from the constructor
There was a problem hiding this comment.
This one is related to the issue I opened qc-lab/QHyper#57 and to #10 (comment). Please take a look and read it carefully.
I'd like to ask you for help with this one.
Originally, in the HierarchicalSampler interface .sample_qubo_to_dict(self) has no args, only self:
So I added this flag "artificially" to the AdvantageSampler and do a check in the HierarchicalSearcher:
.
Is it ok? Wanted to keep it as a bool (True) or None just for safety so that when some other method/class calls Advantage.sample_qubo_to_dict(), the empty brackets would mean a None and would keep the interface safe.
Could you please suggest a better sofware engineering approach to it?
Qommunity/searchers/hierarchical_searcher/hierarchical_searcher.py
Outdated
Show resolved
Hide resolved
Qommunity/searchers/hierarchical_searcher/hierarchical_searcher.py
Outdated
Show resolved
Hide resolved
Qommunity/searchers/hierarchical_searcher/hierarchical_searcher.py
Outdated
Show resolved
Hide resolved
kacper3615
left a comment
kacper3615
left a comment
There was a problem hiding this comment.
I wrote comments next to the proposed changes
|
…asiav/sampleset_times_extraction
…leset_times_extraction.ipynb notebook
2 participants
No description provided.