Pynumero.sparse Updates #1285

michaelbynum · 2020-02-05T20:50:12Z

Summary/Motivation:

This PR makes updates to pyomo.contrib.pynumero.sparse.

Changes proposed in this PR:

Improvements to BlockVector and BlockMatrix
Support for MPIBlockVector and MPIBlockMatrix
More strict dimension requirements for block objects. In particular, once the dimensions for a block are set, they cannot be changed.

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

I agree my contributions are submitted under the BSD license.
I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

- more strict dimension requirements in BlockMatrix - removed BlockSymMatrix and MPIBlockSymMatrix - adding some tests for BlockMatrix - more careful copying within BlockMatrix methods - Simplified BlockMatrix.copyfrom significantly; hopefully these changes will also improve performance, but I don't have any data to support this - Improved caching of block row and block column dimensions within BlockMatrix

michaelbynum · 2020-02-05T20:51:44Z

@santiagoropb Can you also please take a look at this PR?

codecov · 2020-02-05T21:07:52Z

Codecov Report

Merging #1285 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1285   +/-   ##
=======================================
  Coverage   69.96%   69.96%           
=======================================
  Files         536      536           
  Lines       82759    82759           
=======================================
  Hits        57903    57903           
  Misses      24856    24856

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 33f19a0...33f19a0. Read the comment docs.

santiagoropb · 2020-02-06T12:58:04Z

@michaelbynum sure! I'll take a look during the weekend. From a quick look looks great! Thank you! One quick thing I noticed is that test_nlp_composition.py was partially updated to call toarray instead of todense. That file is probably not run after Carls refactor but if you want to keep it there I suggest to update the set and get block notation too.

santiagoropb

This looks overall pretty good! I had some minor comments regarding slicing and matrix products.

One thing that I always wonder is if we could have a decorator on the blockvector and blockmatrix classes to use numba just-in-time compiling. The decorator should check if numba is installed and if so it should just-in-time compile. That could potentially speed up all the python loops we have every time we go through the blocks. The additional work to support that seems very minor. We can also add numba to the conda recipe to always have it installed.

santiagoropb · 2020-02-08T18:10:56Z

pyomo/contrib/pynumero/examples/structured/tests/test_nlp_compositions.py

@@ -961,7 +961,7 @@ def test_jacobian_g(self):
            B2[i, i] = 1.0
        Ji[1, 0] = coo_matrix(B1)
        Ji[1, 1] = coo_matrix(B2)


You may want to change these to set_block across this whole file

pyomo/contrib/pynumero/sparse/__init__.py

santiagoropb · 2020-02-08T18:19:31Z

pyomo/contrib/pynumero/sparse/block_vector.py

-    Structured Vector interface
+    Structured vector interface. This interface can be used to
+    performe operations on vectors composed by vectors. For example
+    bv = BlockVector([v1, v2, v3]), where vi are numpy.ndarrays or BlockVectors.


This doc string is out of date. After your changes we no longer have a constructor that takes a list of blocks

@santiagoropb Good catch. Sorry I removed this functionality from the constructor, but it made MPIBlockVector a lot simpler.

Let me clarify. It was simpler to not allow this functionality in MPIBlockVector and I wanted the API to be consistent across all of the block vectors and matrices.

pyomo/contrib/pynumero/sparse/block_vector.py

santiagoropb · 2020-02-08T18:26:08Z

pyomo/contrib/pynumero/sparse/block_vector.py

+            if x1.__class__.__name__ == 'MPIBlockVector':
+                raise RuntimeError('Operation not supported by BlockVector')
+            if x2.__class__.__name__ == 'MPIBlockVector':
+                raise RuntimeError('Operation not supported by BlockVector')
            raise NotImplementedError()



I like how you call now the assert_block_structure method. Just a side comment. The asserts can be turned of with python -0. That can speed up the code since some block asserts can be expensive. Specially the block matrix ones

With the stricter dimension requirements, I was able to make all of the assert_block_structure methods quite fast. All it does is check the length of a set.

santiagoropb · 2020-02-09T13:41:14Z

pyomo/contrib/pynumero/sparse/block_vector.py

-            self._block_mask[key] = True
-            self._brow_lengths[key] = value.size
+    def __getitem__(self, item):
+        raise NotImplementedError('BlockVector does not support __getitem__.')


I think we can still support this as it was a normal array. We should check if all blocks are defined, then given the "flat index" we identify which block contains the index and we return the value. If the block is a BlockVector we recurse

The tricky part may be when the user passes a slice. We can raise an exception or as in the scalar case gather elements from each block and put them into a numpy array

I agree. I suggest we add an issue and revisit this later.

I created a new issue.

santiagoropb · 2020-02-09T13:42:35Z

pyomo/contrib/pynumero/sparse/block_vector.py


-    def __le__(self, other):
+    def __setitem__(self, key, value):


Similar comments as in getitem

pyomo/contrib/pynumero/sparse/block_matrix.py

santiagoropb · 2020-02-09T14:10:04Z

pyomo/contrib/pynumero/sparse/tests/test_block_matrix.py

+        prod = m * m2
+
+        self.assertIsInstance(prod, BlockMatrix)
+        self.assertTrue(np.allclose(flat_prod.toarray(), prod.toarray()))


If we decide to support product with numpy array we should add a test here

an easy way to support multiplication with 2d arrays would be to call the constructor of scipy.sparse on the array to get a "sparse copy" and use that sparse copy within our multiplication. This is important because some times scipy returns matrices of only one entry as 2d arrays.

we should add a test anyway to make sure the correct error is thrown.

santiagoropb · 2020-02-09T14:15:09Z

pyomo/contrib/pynumero/sparse/mpi_block_matrix.py

+        if isinstance(other, MPIBlockVector):
+            raise NotImplementedError('Vector-Matrix multiply not supported yet')
+        if isinstance(other, BlockVector):
+            raise NotImplementedError('Vector-Matrix multiply not supported yet')


At some point @carldlaird mentioned we could get some performance benefits if we support xTA. Not sure if thats still in his plans?

We should support this when possible. Currently, we are doing a transpose (which copies) of the matrix which could be expensive. Can we add an issue to revisit?

I added an issue.

carldlaird

Most of these can be pushed to discussion, todo, or a new issue. The API changes and comment changes should be done - hopefully the "easy" things can be addressed now.

Feel free to add a single issue with a list of things that were not yet addressed.

pyomo/contrib/pynumero/sparse/base_block.py

pyomo/contrib/pynumero/sparse/block_matrix.py

carldlaird · 2020-02-12T15:02:07Z

pyomo/contrib/pynumero/sparse/mpi_block_vector.py

+
+        return result
+
+    def make_local_copy(self):


same question as above. I think I made a comment about wanting this on MPIBlockMatrix.

I created an issue for this.

carldlaird · 2020-02-12T15:02:47Z

pyomo/contrib/pynumero/sparse/mpi_block_vector.py

+        self._set_block_size(key, value.size)
+
+    def __getitem__(self, item):
+        raise NotImplementedError('MPIBlockVector does not support __getitem__.')


Should we point the user to get_block and set_block in these error messages?

carldlaird · 2020-02-12T15:03:37Z

pyomo/contrib/pynumero/sparse/mpi_block_vector.py

+                # checks only the mask of one of them since all must have the same
+                global_mask[bid] = processor_to_mask[0][bid]
+
+            disp_owner = self._rank_owner[bid] if self._rank_owner[bid] >= 0 else 'A'


Just think it is better to be explicit rather than relying on "codes" to interpret the output.

carldlaird · 2020-02-12T15:04:18Z

pyomo/contrib/pynumero/sparse/tests/test_block_matrix.py

+        prod = m * m2
+
+        self.assertIsInstance(prod, BlockMatrix)
+        self.assertTrue(np.allclose(flat_prod.toarray(), prod.toarray()))


we should add a test anyway to make sure the correct error is thrown.

carldlaird · 2020-02-12T15:05:10Z

pyomo/contrib/pynumero/sparse/warnings.py

@@ -0,0 +1,4 @@
+
+
+class MPISpaceWarning(Warning):


Was this used anywhere?

Good question. I don't think so. I'll remove it.

michaelbynum · 2020-02-20T17:44:32Z

@carldlaird and @santiagoropb I think a addressed all of your concerns. Please have a look.

pyomo/contrib/pynumero/sparse/block_matrix.py

santiagoropb and others added 20 commits January 9, 2020 13:45

updates to pynumero.sparse

3a235a5

updates to BlockMatrix and BlockMatrix tests

79cf7a2

Merge branch 'master' into pynumero_sparse

adf8267

updating BlockVector

fe90577

Merge branch 'master' into pynumero_sparse

9d134d4

more strict dimension requirements for BlockVector

cf5614a

removing coo.py

fc57f6c

updates to MPIBlockMatrix (more strict dimension requirements)

6a4026d

updates to MPIBlockMatrix

b0ada12

BlockMatrix cleanup

6670551

improving a couple loops in BlockMatrix

3826db7

switching from __getitem__ and __setitem__ to get_block and set_block

f13e87b

switching from __getitem__ and __setitem__ to get_block and set_block

9b2ffa2

switching from __getitem__ and __setitem__ to get_block and set_block

2c86b00

merging master into pynumero_sparse

55d97c5

updates to pynumero.sparse

4066a5e

updates to pynumero.sparse

013186c

adding parallel tests for pynumero

b24cad9

typo

5d53f7b

michaelbynum requested review from blnicho and carldlaird February 5, 2020 20:51

adding a workflow for parallel tests

0a80516

michaelbynum added 5 commits February 5, 2020 14:11

updating parallel tests workflow

29bac5a

updating parallel_tests workflow

038f2f1

fixing division

178b74f

fixing division with BlockMatrix

f6c0dde

fixing import

1c82820

fixing division for python 2.7

e70f180

santiagoropb reviewed Feb 9, 2020

View reviewed changes

jsiirola mentioned this pull request Feb 11, 2020

Benders MPI logic bug and parallel test coverage #1278

Merged

carldlaird requested changes Feb 12, 2020

View reviewed changes

michaelbynum added 2 commits February 20, 2020 06:20

merging master into pynumero_sparse

7665068

addressing PR comments for pynumero.sparse updates

3ea3340

michaelbynum added 2 commits February 20, 2020 11:07

updating tests

27a7995

allowing numpy arrays in BlockMatrix.set_block, but with a warning

33f19a0

carldlaird approved these changes Mar 3, 2020

View reviewed changes

pyomo/contrib/pynumero/sparse/block_matrix.py Show resolved Hide resolved

blnicho merged commit 6fd23bd into Pyomo:master Mar 3, 2020

jsiirola mentioned this pull request Mar 3, 2020

Standardizing github actions naming #1323

Merged

michaelbynum deleted the pynumero_sparse branch March 5, 2020 18:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pynumero.sparse Updates #1285

Pynumero.sparse Updates #1285

michaelbynum commented Feb 5, 2020

michaelbynum commented Feb 5, 2020

codecov bot commented Feb 5, 2020 •

edited

Loading

santiagoropb commented Feb 6, 2020

santiagoropb left a comment

santiagoropb Feb 8, 2020

santiagoropb Feb 8, 2020

michaelbynum Feb 20, 2020

michaelbynum Feb 20, 2020

santiagoropb Feb 8, 2020

michaelbynum Feb 20, 2020

santiagoropb Feb 9, 2020

santiagoropb Feb 9, 2020

carldlaird Feb 12, 2020

michaelbynum Feb 20, 2020

santiagoropb Feb 9, 2020

santiagoropb Feb 9, 2020

santiagoropb Feb 9, 2020

carldlaird Feb 12, 2020

santiagoropb Feb 9, 2020

carldlaird Feb 12, 2020

michaelbynum Feb 20, 2020

carldlaird left a comment

carldlaird Feb 12, 2020

michaelbynum Feb 20, 2020

carldlaird Feb 12, 2020

michaelbynum Feb 20, 2020

carldlaird Feb 12, 2020

carldlaird Feb 12, 2020

carldlaird Feb 12, 2020

michaelbynum Feb 20, 2020

michaelbynum commented Feb 20, 2020

Pynumero.sparse Updates #1285

Pynumero.sparse Updates #1285

Conversation

michaelbynum commented Feb 5, 2020

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

michaelbynum commented Feb 5, 2020

codecov bot commented Feb 5, 2020 • edited Loading

Codecov Report

santiagoropb commented Feb 6, 2020

santiagoropb 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

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

carldlaird 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelbynum commented Feb 20, 2020

codecov bot commented Feb 5, 2020 •

edited

Loading