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

C++ BQM.add_quadratic from COO #819

Conversation

arcondello
Copy link
Member

No description provided.

@arcondello arcondello added the enhancement New feature or request label May 4, 2021
@arcondello arcondello changed the title Feature/c++ bqm.add quadratic from coo C++ BQM.add_quadratic from COO May 4, 2021
@codecov-commenter
Copy link

codecov-commenter commented May 4, 2021

Codecov Report

Merging #819 (4f90494) into main (1d13475) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #819   +/-   ##
=======================================
  Coverage   92.50%   92.50%           
=======================================
  Files          63       63           
  Lines        4658     4658           
=======================================
  Hits         4309     4309           
  Misses        349      349           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d13475...4f90494. Read the comment docs.

dimod/include/dimod/utils.h Outdated Show resolved Hide resolved
dimod/include/dimod/utils.h Show resolved Hide resolved
dimod/include/dimod/quadratic_model.h Outdated Show resolved Hide resolved
dimod/include/dimod/quadratic_model.h Outdated Show resolved Hide resolved
} else if (length < 0) {
throw std::out_of_range("length must be positive");
}

Choose a reason for hiding this comment

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

Do you want to do an early exit if length == 0 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The solution works for length==0 as-is and I don't think it's a case worth optimizing for.


std::int64_t pivot_choice_index = (low + high + 1) / 2;
if (pivot_choice_index != high) {
T2 response_temp = response[pivot_choice_index];

Choose a reason for hiding this comment

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

Sorry I may have not made my comment clear enough, we need to have the old code and the new code, that is whenever you swap the control you need to swap the response, I had been missing the response part.

Copy link
Member Author

Choose a reason for hiding this comment

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

this is what I get for being lazy and not adding tests 😆 . Ok I will do it the right way...

/// Return the size of the neighborhood.
size_type size() const { return neighbors.size(); }

/// Sort the neighborhood and sum the biases of duplicate variables.
void sort_and_sum() {
Copy link

@amahmudwave amahmudwave May 5, 2021

Choose a reason for hiding this comment

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

Do you want to move this i mean the part of code to do the sum only to the utils like the zip sort one ? Up to 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.

Eh, for now I'll leave it be and move it when it becomes relevant.

Co-authored-by: Anil Mahmud <amahmud@dwavesys.com>
@arcondello arcondello force-pushed the feature/C++-BQM.add_quadratic-from-coo branch from 69296d8 to 40bbe65 Compare May 5, 2021 18:10
@arcondello arcondello merged commit 0d73fc6 into dwavesystems:main May 5, 2021
@arcondello arcondello deleted the feature/C++-BQM.add_quadratic-from-coo branch May 5, 2021 18:50
arcondello added a commit that referenced this pull request May 7, 2021
New Features
------------
* Support PEP518 for building the package #814
* Add `cyVariables.size()` and `.at(..)` to improve cython access to the `Variables` object
* Add new `QuadraticModelBase` and `BinaryQuadraticModel` implementation in c++ #818, #819

Fixes
-----
* Fix broken documentation links #815
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants