Condense distance matrix that was not. Pep8 + test pass #1084
Conversation
|
Passes test, but no validation that it doesn't change the result. It shouldn't.. but maybe have @frheault validate the fix since I think he's more familiar with spatial manipulations |
scilpy/connectivity/matrix_tools.py
Outdated
| @@ -36,9 +37,11 @@ def compute_olo(array): | |||
| if array.ndim != 2: | |||
| raise ValueError('RCM can only be applied to 2D array.') | |||
|
|
|||
| Z = hierarchy.ward(array) | |||
| condensed_array = squareform(array) | |||
There was a problem hiding this comment.
I don't understand why squareform would help here? A connectivity array (like streamline count) is already a form of distance matrix. I think it is fine as it was, no?
What was your goal when you found and added squareform?
There was a problem hiding this comment.
When trying to solve this issue, I learned that a distance matrix is symmetric and can be condensed into a a list or vector. Similarly to a graph adjacency matrix that you transform into an adjacency list.
The error was that the matrix looked "suspiciously like an uncondensed distance matrix" when used in the optimal_leaf_ordering function. Using a symmetric (redundant) distance matrix in scipy functions gives a warning because it's suboptimal. (I suppose)
Squareform converts a uncondensed distance matrix into a condensed one and vice versa. The error disapeared after that.
There was a problem hiding this comment.
Ok but our matrices is real data, we can't change it to fix an error. Did you verify if the input/output still made sense?
Otherwise I would leave it like that.
There was a problem hiding this comment.
I'm not sure what you mean by real data. Maybe we should walk about it in person. 😄
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1084 +/- ##
==========================================
+ Coverage 69.45% 69.46% +0.01%
==========================================
Files 447 447
Lines 24025 24054 +29
Branches 3280 3291 +11
==========================================
+ Hits 16687 16710 +23
- Misses 5943 5946 +3
- Partials 1395 1398 +3
|
|
Okay after discussion, squareform seems to result in the wrong permutations because there is data on the diagonal of the matrix that gets lost when condensed. Works with test data, but not with real data. We instead will simply mute the warning, as it's not a big deal. The warning was already set to be muted in matrix_tools.py, but somehow it didn't work with the tests. So I added another line in the problematic test case to mute it again. Now we don't see it. |
|
Adding the simplefilter at the top of the test script does not seem to work. This is why I put it inside the test case. |
Quick description
Closes #1060
Type of change
Check the relevant options.
Provide data, screenshots, command line to test (if relevant)
...
Checklist