Ported DQM code to C++ #781

amahmudwave · 2021-01-24T01:20:07Z

Ported all the dqm code to C++. Not making any attempt to rebase with upstream/main since there is a lot of divergence now.

amahmudwave · 2021-01-24T01:22:15Z

dimod/discrete/cydiscrete_quadratic_model.pyx


-                self.bqm_.set_quadratic(cu, cv, bias)
+            self.dqm_.connect_variables(u,v)


@arcondello I put this line of code only for a test case to pass, I think I should remove it and modify the test case, instead

350 dqm.set_quadratic('a', 'b', {}) 351 352 self.assertEqual(dqm.get_quadratic('a', 'b'), {})

since if we serialize this dqm, and de-serialize differences will arise, as connections among variables are recreated based on actual connections among cases. I can fix it in this PR or in a different PR (since this changes the original behaviour of the code, so far it just imitates)

Right, I know I told you the old behavior was a bug, but I think it is in fact meaningful for dqm.set_quadratic(u, v, {}) to set the interaction, but not biases. I know it's weird, but I suppose I can imagine a case where I populate the structure of the DQM before setting the biases where I would want to do that.

Okay I will keep it as it is, but we could always end up making at test case that fails due to this issue. We serialize with redundant connection, and the connection is not present in the deserialized one.

amahmudwave · 2021-01-24T01:37:50Z

@arcondello This is ready for review, I guess we need some co-ordination to get it merged. I think merging will be better than rebasing in this case.

arcondello · 2021-01-25T17:35:48Z

dimod/include/dimod/adjvectordqm.h

+
+    AdjVectorDQM() { case_starts_.push_back(0); }
+
+    explicit AdjVectorDQM(const AdjVectorDQM &dqm) {


This is probably a basic C++ question, but will this handle DQMs that are not of the same template type?

I have to dig deeper into it. I followed the AdjVectorBQM, which does use a template over here, but in the cppbqm.pxd file it has only the declaration for something like AdjVectorBQM(cons AdjVectorBQM & bqm), we are not passing a template parameter for specifying the type of the other BQM.

Templated it the way adjvectorbqm copy constructor is templated.

arcondello · 2021-01-25T17:38:06Z

dimod/include/dimod/adjvectordqm.h

+    }
+
+    template <class io_variable_type, class io_bias_type>
+    void extract_data(io_variable_type *case_starts,


I think to_coo or something like that would be a better name.

Alternatively, since this is just reading the DQM, I think it makes sense to leave this code in cython.

arcondello · 2021-01-25T17:38:50Z

dimod/include/dimod/adjvectordqm.h

+        return connected;
+    }
+
+    void connect_variables(variable_type u, variable_type v) {


should this be private and/or protected?

I made it private initially but the problem is for the bug I mentioned above this is needed, to create the connection in cython.

arcondello · 2021-01-25T17:38:55Z

dimod/include/dimod/adjvectordqm.h

+        return false;
+    }
+
+    bool connection_present(variable_type u, variable_type v) {


should this be private and/or protected?

This needs to be used inside cython, to raise value error, when no connection is present.

arcondello · 2021-01-25T18:01:58Z

dimod/include/dimod/adjvectordqm.h

+        return v;
+    }
+
+    bias_type get_linear_case(variable_type v, variable_type case_v) {


In the bqm I am switching to bias_type& linear(v) and I think bias_type& linear_case(u, v) would also be preferable to get/set.

Note that for setting to work with cython, you need some trickery. E.g.

cdef bias_type *b = &(self.cppbqm.linear(v)) b[0] = bias

In python there isn't really a notion of a reference/proxy object in the same way. So we'll want to keep get/set.

arcondello · 2021-01-25T18:35:56Z

dimod/include/dimod/adjvectordqm.h

+    }
+
+    template <class io_variable_type, class io_bias_type>
+    void get_energies(io_variable_type *samples, int num_samples,


IMO it makes sense to just have a single method, bias_type energy(Iter sample_start). The user can then parallelize it themselves if they like.

arcondello · 2021-01-25T18:37:16Z

dimod/include/dimod/adjvectordqm.h

+    }
+
+    template <class io_bias_type>
+    bool set_quadratic(variable_type u, variable_type v, io_bias_type *biases) {


I'd rather accept an iterator than a pointer (applies other places). Let me know if you disagree.

If we pass in an iterator while passing a pointer we are sort of hiding the real information, we can always use the pointer as an iterator. The thing is we might end up doing pointer arithmetic which does not look too good on an iterator.

I guess it seems to me that the iterator is the more general object. Specifically here we mean a random access iterator, since as you say we might end up doing arithmetic. But using an iterator instead would allow us to use strided arrays for instance.

Let me think about it a bit, this is also coming up in the BQM case.

arcondello · 2021-01-25T18:39:34Z

dimod/discrete/cydiscrete_quadratic_model.pyx


-                self.bqm_.set_quadratic(cu, cv, bias)
+            self.dqm_.connect_variables(u,v)


Right, I know I told you the old behavior was a bug, but I think it is in fact meaningful for dqm.set_quadratic(u, v, {}) to set the interaction, but not biases. I know it's weird, but I suppose I can imagine a case where I populate the structure of the DQM before setting the biases where I would want to do that.

arcondello · 2021-01-25T18:39:46Z

dimod/include/dimod/adjvectordqm.h

+#ifndef DIMOD_ADJVECTORDQM_H_
+#define DIMOD_ADJVECTORDQM_H_
+
+#include <stdio.h>


Is this used?

codecov-io · 2021-01-25T21:50:46Z

Codecov Report

Merging #781 (2e5e4c8) into main (f4d2744) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #781   +/-   ##
=======================================
  Coverage   91.81%   91.81%           
=======================================
  Files          63       63           
  Lines        4667     4667           
=======================================
  Hits         4285     4285           
  Misses        382      382

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 f4d2744...2e5e4c8. Read the comment docs.

amahmudwave

Addressed the review comments, regarding taking to_coo (previously extract_data) , if we want to parallelize it, it will be a lot of code, so I think its better to keep inside c++ instead of cython.

amahmudwave · 2021-01-25T19:02:48Z

dimod/discrete/cydiscrete_quadratic_model.pyx


-                self.bqm_.set_quadratic(cu, cv, bias)
+            self.dqm_.connect_variables(u,v)


Okay I will keep it as it is, but we could always end up making at test case that fails due to this issue. We serialize with redundant connection, and the connection is not present in the deserialized one.

amahmudwave · 2021-01-25T21:49:03Z

dimod/include/dimod/adjvectordqm.h

+
+    AdjVectorDQM() { case_starts_.push_back(0); }
+
+    explicit AdjVectorDQM(const AdjVectorDQM &dqm) {


I have to dig deeper into it. I followed the AdjVectorBQM, which does use a template over here, but in the cppbqm.pxd file it has only the declaration for something like AdjVectorBQM(cons AdjVectorBQM & bqm), we are not passing a template parameter for specifying the type of the other BQM.

amahmudwave · 2021-01-25T21:49:41Z

dimod/include/dimod/adjvectordqm.h

+        return false;
+    }
+
+    bool connection_present(variable_type u, variable_type v) {


This needs to be used inside cython, to raise value error, when no connection is present.

amahmudwave · 2021-01-25T21:50:07Z

dimod/include/dimod/adjvectordqm.h

+        return connected;
+    }
+
+    void connect_variables(variable_type u, variable_type v) {


I made it private initially but the problem is for the bug I mentioned above this is needed, to create the connection in cython.

amahmudwave · 2021-01-25T21:51:22Z

dimod/include/dimod/adjvectordqm.h

+    }
+
+    template <class io_bias_type>
+    bool set_quadratic(variable_type u, variable_type v, io_bias_type *biases) {


If we pass in an iterator while passing a pointer we are sort of hiding the real information, we can always use the pointer as an iterator. The thing is we might end up doing pointer arithmetic which does not look too good on an iterator.

Anil Mahmud added 30 commits January 12, 2021 14:45

Initial commit, added header file for dqm

c5872e0

Added the energy function

b916187

Added more functions

3dc630d

Refinements non-specific

e98a393

Finished dqm basic functions except to-from conversion

2497a30

Changed names to be compatible with current cython code

d66cb30

NFC

743bcac

fixes

e87373f

Fix asserts & parallelize quadratic operations

4cf6687

Fix parallelization bug

fcebf05

Added functions for converting from numpy vectors

b4a983e

Added declaration for adjvectordqm.h in cython

bfea932

Update the pxd file for dqm for type names

ce9ed8b

Initial cython changes

a51c270

Templated from numpy function

9e160c0

Templated the functions.

6d76a93

Manual rebase to master

89fef86

Updated cydiscrete pyx file

55809b0

Fix pxd file

aa12593

Completed initial to_numpy_vectors

42d03fe

NFC & type fix

3ceafa9

Fix bug missing arg

6fb1510

NFC and add new function to test connection

946d508

Reflect changes in cpp file in pxd file

622117a

Fixed most cython compilation errors

194d4e8

Fixed a bunch of errors

7cd854a

Fixed bug

62bbd13

Fix auto related errors

49e476c

Remove auto as they are not safe for immediate assignments

86f8056

Fix bugs

6ad650d

Anil Mahmud added 3 commits January 22, 2021 19:26

Fix bugs

9372616

Fix bug

36ee4cf

Fix bugs

34769c1

amahmudwave commented Jan 24, 2021

View reviewed changes

arcondello reviewed Jan 25, 2021

View reviewed changes

Anil Mahmud added 2 commits January 25, 2021 13:14

Addressed some review comments

5e324b3

Switched from get/set linear case to linear_case

2e5e4c8

Made copy constructor templated

0e75ca9

amahmudwave commented Jan 25, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ported DQM code to C++ #781

Ported DQM code to C++ #781

amahmudwave commented Jan 24, 2021

amahmudwave Jan 24, 2021

arcondello Jan 25, 2021

amahmudwave Jan 25, 2021

amahmudwave commented Jan 24, 2021

arcondello Jan 25, 2021

amahmudwave Jan 25, 2021

amahmudwave Jan 25, 2021

arcondello Jan 25, 2021

arcondello Jan 25, 2021

arcondello Jan 25, 2021

amahmudwave Jan 25, 2021

arcondello Jan 25, 2021

amahmudwave Jan 25, 2021

arcondello Jan 25, 2021

arcondello Jan 25, 2021

arcondello Jan 25, 2021

amahmudwave Jan 25, 2021

arcondello Jan 25, 2021

arcondello Jan 25, 2021

arcondello Jan 25, 2021

arcondello Jan 25, 2021

amahmudwave Jan 25, 2021

codecov-io commented Jan 25, 2021 •

edited

Loading

amahmudwave left a comment

amahmudwave Jan 25, 2021

amahmudwave Jan 25, 2021

amahmudwave Jan 25, 2021

amahmudwave Jan 25, 2021

amahmudwave Jan 25, 2021


		self.bqm_.set_quadratic(cu, cv, bias)
		self.dqm_.connect_variables(u,v)


		AdjVectorDQM() { case_starts_.push_back(0); }

		explicit AdjVectorDQM(const AdjVectorDQM &dqm) {

Ported DQM code to C++ #781

Are you sure you want to change the base?

Ported DQM code to C++ #781

Conversation

amahmudwave commented Jan 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amahmudwave commented Jan 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 25, 2021 • edited Loading

Codecov Report

amahmudwave left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 25, 2021 •

edited

Loading