-
Notifications
You must be signed in to change notification settings - Fork 41
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
Random point for tangent space #111
Random point for tangent space #111
Conversation
…lidean manifolds * use representation_size instead manifold_dimension
Codecov Report
@@ Coverage Diff @@
## master #111 +/- ##
=======================================
Coverage 99.63% 99.63%
=======================================
Files 47 47
Lines 3023 3027 +4
=======================================
+ Hits 3012 3016 +4
Misses 11 11
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
Sure sounds reasonable (though simple), could you check for the 4 lines missing in code coverage, i.e. also write tests for your new functions? The docs look okay, but could also be a little longer. |
I am getting an error when I access the code coverage report: https://app.codecov.io/gh/JuliaManifolds/Manopt.jl/compare/111?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr%20comments&utm_term=JuliaManifolds No idea what's happening there, because I do have tests for the new functions (https://github.com/JuliaManifolds/Manopt.jl/pull/111/files#diff-aa4320949f987b556afb5c0fbfb0e95e09a8c07bf72fc82aa478196179db22acR58-R65). I wonder if it is related to the fact that this PR builds upon #110. |
I will see if I can add a bit more text tomorrow :) |
Does the direct comparison link work? It seems for now your new functions are not called Also you have to merge master once to here in order to be able to merge this later on. |
Thanks for the contribution! The coverage is not detected because your new tests are in a file that is not run during CI. You need to |
Ah that I did not see that fast then even on the current master these are not run – but luckily now they are :) |
this one is nearly ready to merge, we are just missing code coverage for the 4 new lines it seems? |
Great. I will take a look tomorrow :) |
(Sorry for the little delay) if you could bump the version to 0.3.19 we could also directly register this so its available for everyone who updates. |
I wrote a
random_point()
andrandom_tangent()
function for theTangentSpaceAtPoint
manifolds. Both are pretty straightforward, as they both simply callrandom_tangent()
for the original manifold. Let me know if you think that could be a useful contribution.