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

Add BinaryQuadraticModel.add_bqm to C++ BQM #821

Conversation

arcondello
Copy link
Member

No description provided.

@arcondello arcondello requested a review from amahmudwave May 11, 2021 18:42
@arcondello
Copy link
Member Author

CI failure is unrelated to this PR.

*
* The size of the updated BQM will be adjusted appropriately.
*
* If the other BQM does not have the same vartype, the biases are adjusted

Choose a reason for hiding this comment

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

if the other, -> what does other refer to ? A more specific term may be used.

*/
template <class B, class I>
void add_bqm(const BinaryQuadraticModel<B, I>& bqm) {
if (bqm.vartype() != vartype()) {

Choose a reason for hiding this comment

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

in such places a this-> looks better, aesthetic stuff , upto you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. There are lots of places to do this, I'll keep it consistent for now and then take a pass later adding them in.


base_type::adj_[v].reserve(base_type::adj_[v].size() +
bqm.adj_[v].size());
for (auto it = bqm.adj_[v].cbegin(); it != bqm.adj_[v].cend();

Choose a reason for hiding this comment

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

You can use the std::insert, it may use some optimizations which are not exposed to the user, for example doing a memcpy and then increasing the size directly, depends on the implementation. If you care about aesthetics, after you use std::insert you can benchmark with and without the above reserve since std::insert might just be doing that itself if needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would need to implement insert on the neighbor object, which I could so anyway to abstract this.

Copy link

@amahmudwave amahmudwave May 11, 2021

Choose a reason for hiding this comment

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

Oh I thought this was a vector, nevermind then. Since you would not have that lowlevel control over the elements of the neighborhood , I mean doing memcpy, etc., I mean you can do memcpy but you cannot just set the size arbitrarily without doing resize which just defeats the purpose.


base_type::adj_[v].reserve(base_type::adj_[v].size() +
bqm.adj_[v].size());
for (auto it = bqm.adj_[v].cbegin(); it != bqm.adj_[v].cend();

Choose a reason for hiding this comment

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

Same comment as above.

@arcondello arcondello merged commit e17f6f2 into dwavesystems:main May 11, 2021
@arcondello arcondello deleted the feature/C++-BinaryQuadraticModel.add_bqm branch May 11, 2021 19:02
arcondello added a commit that referenced this pull request Jun 11, 2021
New Features
------------

* Add `add_bqm` method to C++ BinaryQuadraticModel #821, #823
* Add `Structured.valid_bqm_graph` method for verifying input problem structure #832
* Reimplement `BinaryQuadraticModel` to use new C++ code #828
* `BinaryQuadraticModel` can now be manipulated symbolically #834
* `load` function can now load all model types #841, #843
* `DiscreteQuadraticModel` now has an `.offset` attribute #838
* Add `ConstrainedQuadraticModel` class #839

Fix
---
* Fix type promotions in binary quadratic models with object biases #836
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.

2 participants