-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplify project! for Stiefel #153
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Can you check that the tests pass locally for you? (the CI is currently failing due to other issues). If so, then I think this is good to merge.
Better yet, now master should just work, so merge master in, and if it passes, I'll merge the PR. |
Oh nevermind, I see that the CI is running correctly after reopening, it's just that the checks also show the old failed CI because those have different names. Okay, we'll just see if it passes. |
Codecov Report
@@ Coverage Diff @@
## master #153 +/- ##
==========================================
- Coverage 96.72% 96.71% -0.01%
==========================================
Files 48 48
Lines 3023 3015 -8
==========================================
- Hits 2924 2916 -8
Misses 99 99
Continue to review full report at Codecov.
|
Wow, you are quite efficient. Thanks for resolving the testing stuff and merging! :) |
Thanks for helping and contributing :) |
Do simplifications of
project!
method discussed in #141 and mentioned in #142.Key idea is that
s.U' * s.U
in the previous implementation should (up to numerical issues) be approximatelyI
in which case the remaining code simplifies.