Implement QuadraticModelBase and BinaryQuadraticModel in C++ #818
Conversation
QuadraticModelBase can be subclassed in the future to encode more generic types of quadratic models - with the vartype tracked per variable.
Codecov Report
@@ Coverage Diff @@
## main #818 +/- ##
=======================================
Coverage 92.50% 92.50%
=======================================
Files 63 63
Lines 4658 4658
=======================================
Hits 4309 4309
Misses 349 349 Continue to review full report at Codecov.
|
| value_type operator*() { return value_type{*neighbor_it_, *bias_it_}; } | ||
|
|
||
| private: | ||
| bias_iterator bias_it_; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
In c++ we follow google's style guide. In python you're right, the convention is to use a leading _. And cython who knows 😆
There was a problem hiding this comment.
Once an arguments name was say name, and the member was name_, i used name instead of name_, should not have happened if i had used _nameinstead of name_, this is an isolated rare case though :)
| std::vector<bias_type> quadratic_biases; | ||
| }; | ||
|
|
||
| template <class Bias, class Neighbor = std::int64_t> |
There was a problem hiding this comment.
I think the default type should be just, int, reasons.
-
We save a huge amount of memory now that the vectors are not vectors of pairs and structures are not padded, so if we use bias type float using int will save 1/3rd memory and if bias type is double then it samves 1/4rth memory.
-
We will never get close to using that many variables, and by the time we use that many variables we have to change the algorithms we use for adding biases together since the drift will be too large to handle.
-
Moving to int from int_64t will not cause any precision bug until and unless we somehow use it in bias computation - something that I did and suffered lol.
| using bias_type = Bias; | ||
|
|
||
| /// The type for variable indices | ||
| using index_type = std::ptrdiff_t; |
There was a problem hiding this comment.
I do not think we need a separate type for indexing, I mean class Neighbor could be just index_type, we will not index to anything that is larger than Neighbor , the only thing that may be larger than what Neighbor type can hold is num_interactions, so having separate index_type and Neighbor is just for aesthetic reasons, and I do not have any argument against that if it is not hard to remember and be consistent with the types.
One question,will it be easy for you to declare these functions which use index_type in cython ?
There was a problem hiding this comment.
Ok, maybe I will just drop neighbor_type altogether and just use index_type to simplify it. And yes, cython can handle it just fine.
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
First of several PRs breaking up one big one. This will replace the other dimod BQM C++ implementation. Also can be easily sub-classed for other types of quadratic models.