JuliaManifolds / ManifoldsBase.jl

Basic interface for manifolds in Julia
https://juliamanifolds.github.io/ManifoldsBase.jl/
MIT License
87 stars 8 forks source link

remove unused type variables #133

Closed mateuszbaran closed 1 year ago

mateuszbaran commented 1 year ago

Resolves #132. Manifolds.jl has the same issue in a few places which have to be resolved in another PR.

codecov[bot] commented 1 year ago

Codecov Report

Merging #133 (a299890) into master (44c6c21) will decrease coverage by 0.08%. The diff coverage is 98.00%.

@@            Coverage Diff             @@
##           master     #133      +/-   ##
==========================================
- Coverage   99.82%   99.73%   -0.09%     
==========================================
  Files          18       18              
  Lines        2247     2297      +50     
==========================================
+ Hits         2243     2291      +48     
- Misses          4        6       +2     
Impacted Files Coverage Δ
src/point_vector_fallbacks.jl 99.24% <98.00%> (-0.76%) :arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

mateuszbaran commented 1 year ago

The one new uncovered line looks like a false positive.

kellertuer commented 1 year ago

Interesting approach! I had thought we would just split Twhere into Point and TVector types and put them were necessary – but that would be breaking. Is this one a breaking change?

kellertuer commented 1 year ago

Besides that, allocate has an unused variable S in there (I removed it on my branch) but you could remove it here as well since it fits this PR very well.

mateuszbaran commented 1 year ago

Interesting approach! I had thought we would just split Twhere into Point and TVector types and put them were necessary – but that would be breaking. Is this one a breaking change?

The problem is we sometimes need the where clause and sometimes we don't, and sadly I couldn't find anything that would interpolate to "this is empty" in type variable list after where (which would simplify things significantly). Luckily this is not breaking.

Besides that, allocate has an unused variable S in there (I removed it on my branch) but you could remove it here as well since it fits this PR very well.

Sure, good idea. Weird that it didn't give me a warning.

kellertuer commented 1 year ago

Great, because mine would have been breaking.

Though I think in your case there might now be hidden unused variables, since Twhere still contains possibly a point and a vector type but there are functions that use only one of them. I am surprised those do not get recognised as warnings.