Added compatibility with correction matrix #61
Conversation
NehaBari
commented
May 20, 2021
NehaBari
commented
- Pulled changes from upstream to forked repo.
- Changed line 234 in _gmlvq.py to accept relevance correction as a parameter.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## fr-51-null-space-correction #61 +/- ##
===============================================================
- Coverage 99.29% 99.01% -0.28%
===============================================================
Files 52 54 +2
Lines 1550 1622 +72
Branches 133 142 +9
===============================================================
+ Hits 1539 1606 +67
- Misses 1 5 +4
- Partials 10 11 +1 ☔ View full report in Codecov by Sentry. |
|
@NehaBari Good work! I changed it to a "draft" pull request, because some things need to happen before it can be accepted: The codecov checks fail, this means that the test coverage of the project goes down and the method should be tested. The tests are located in the module's test folder (i.e., sklvq/models/tests). In the Secondly, could you remove the old code (instead of commenting it out). We don't need the old (wrong) solution in the comments when using version control :-) Thirdly, we should think about including an example in the documentation, i.e., an example that will be shown in sklvq.readthedocs.io. The examples that you see there are located here. |
|
Dear Rick, Thankyou for the detailed feedback. Sounds good. I will start incorporating them. |
Given matrices A and B so that the operations can be pre-formed properties of a transpose
Given matrices A and B so that the operations can be pre-formed properties of a transpose removed some comments
| @@ -385,7 +386,7 @@ def _normalize_omega(omega: np.ndarray) -> None: | |||
|
|
|||
| def _correct_omega(self, omega: np.ndarray) -> None: | |||
| # Note matmul is faster for larger matrices, for smaller dot is faster. | |||
| np.matmul(omega.T, self.relevance_correction, out=omega.T) | |||
| np.matmul(self.relevance_correction,omega,out=omega.T) | |||
There was a problem hiding this comment.
@NehaBari Are you sure this is correct and you did not change the way the correction matrix was defined? Would be good to add this to the tests.
