-
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
Basis number system swap #754
Conversation
I've made the tests for ProjectiveSpace pass but I haven't verified if the code is indeed correct (it's just the same logic as on master). Notably, sphere only has the real coefficient basis. |
I am also not sure the old Euclidean code was correct. But then we should check that. Just that today I am busy (a bit behind with my lecture for tomorrow, and a bit of grading). So I'll probably only tomorrow have time to take a look. |
I haven't found any issues with |
The original example
also threw errors in Euclidean, but I have to check whether your fix in ManifoldsBase already fixed that then. |
No, that wasn't a problem with Euclidean but thanks for reminding me -- there was another issue in a different place that I had to fix to make the example work. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #754 +/- ##
==========================================
- Coverage 99.69% 99.68% -0.01%
==========================================
Files 123 123
Lines 11418 11444 +26
==========================================
+ Hits 11383 11408 +25
- Misses 35 36 +1 ☔ View full report in Codecov by Sentry. |
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.
This looks good to me, if code coverage agrees of course.
Euclidean
ProjectiveSpace
(I'm not sure the old code is even correct for real basis of complex projective space?)SymmetricMatrices