Various small changes #241
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #241 +/- ##
=======================================
Coverage 65.46% 65.46%
=======================================
Files 47 47
Lines 12266 12266
=======================================
Hits 8030 8030
Misses 4236 4236 ☔ View full report in Codecov by Sentry. |
pygeo/parameterization/DVGeo.py
Outdated
| @@ -1520,6 +1520,7 @@ def addCompositeDV(self, dvName, ptSetName=None, u=None, scale=None, prependName | |||
| self.computeTotalJacobian(ptSetName) | |||
| J_full = self.JT[ptSetName].todense() # this is in CSR format but we convert it to a dense matrix | |||
| u, s, _ = np.linalg.svd(J_full, full_matrices=False) | |||
| u = np.array(u) | |||
There was a problem hiding this comment.
The other two changes look good. Can you explain this one a bit more? I thought u was supposed to be an N_DV by N_DV matrix
There was a problem hiding this comment.
Yeah it is, the issue is that it's an N_DV by N_DV np.matrix not np.array. Because of this, values ends up being a (1,N_DV) np.matrix and that breaks pyoptsparse. Calling flatten on a matrix doesn't do anything because the matrix type always wants to remain 2d.
There was a problem hiding this comment.
I don't know why the return is an np.matrix because the docs suggest that the return type should be an array
There was a problem hiding this comment.
@A-CGray This is expected as the input J_full is of a dense matrix type, thus the output will be of a matrix type. The docs you linked do mention this (right before the examples) ,
If a is a matrix object (as opposed to an ndarray), then so are all the return values.
We should probably just change todense() to toarray(), resulting in a dense array as opposed to dense matrix. I dont think there should be any, but make sure that there are no special matrix operations like * (matmul) elsewhere that might be affected.
There was a problem hiding this comment.
Oh, good catch, shows you how observant I am. I agree, will change to toarray and remove this conversion
There was a problem hiding this comment.
@ArshSaja confirmed that he also had to fix this to use composite DVs, so it seems like this was broken from the start
pygeo/parameterization/DVGeo.py
Outdated
| @@ -1520,6 +1520,7 @@ def addCompositeDV(self, dvName, ptSetName=None, u=None, scale=None, prependName | |||
| self.computeTotalJacobian(ptSetName) | |||
| J_full = self.JT[ptSetName].todense() # this is in CSR format but we convert it to a dense matrix | |||
| u, s, _ = np.linalg.svd(J_full, full_matrices=False) | |||
| u = np.array(u) | |||
There was a problem hiding this comment.
@A-CGray This is expected as the input J_full is of a dense matrix type, thus the output will be of a matrix type. The docs you linked do mention this (right before the examples) ,
If a is a matrix object (as opposed to an ndarray), then so are all the return values.
We should probably just change todense() to toarray(), resulting in a dense array as opposed to dense matrix. I dont think there should be any, but make sure that there are no special matrix operations like * (matmul) elsewhere that might be affected.
Purpose
-1e10toNonewhich is the correct way to represent that there is no lower bound.addRefAxisthat is supposed to evaluate to false is x/y/zFraction is None, but currently also evaluates to False is the values are zero.Expected time until merged
Type of change
Testing
Checklist
flake8andblackto make sure the Python code adheres to PEP-8 and is consistently formattedfprettifyor C/C++ code withclang-formatas applicable