DataSlingers / MoMA

MoMA: Modern Multivariate Analysis in R
https://DataSlingers.github.io/MoMA
GNU General Public License v2.0
22 stars 4 forks source link

Deflation for CCA and LDA #46

Closed Banana1530 closed 5 years ago

codecov[bot] commented 5 years ago

Codecov Report

Merging #46 into master will decrease coverage by 13.16%. The diff coverage is 55.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #46       +/-   ##
=========================================
- Coverage   90.17%    77%   -13.17%     
=========================================
  Files          32     34        +2     
  Lines        2372   3558     +1186     
=========================================
+ Hits         2139   2740      +601     
- Misses        233    818      +585
Impacted Files Coverage Δ
R/moma_solve.R 98.07% <ø> (ø)
src/moma.h 100% <ø> (ø) :arrow_up:
src/moma_solver.h 100% <ø> (ø) :arrow_up:
src/moma_base.h 100% <ø> (ø) :arrow_up:
src/moma_solver_BICsearch.cpp 100% <100%> (ø) :arrow_up:
R/util.R 100% <100%> (+4.54%) :arrow_up:
src/moma_solver.cpp 94.73% <100%> (ø) :arrow_up:
R/moma_sflda.R 44.74% <44.74%> (ø)
R/moma_sfpca.R 70.62% <47.59%> (-20.02%) :arrow_down:
R/moma_sfcca.R 56% <56%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 00839b5...ff6da69. Read the comment docs.

michaelweylandt commented 5 years ago

The CodeCov report is excellent! Thanks for adding.

Can we add CodeCov first as a small separate PR so we get reports for all the big PRs before we merge them?

michaelweylandt commented 5 years ago

It looks like some of the commits got misordered: I'm not sure how https://github.com/michaelweylandt/MoMA/pull/46/commits/91b8210eef2c1953bbfbb171e4a9952c8044acdd can follow https://github.com/michaelweylandt/MoMA/pull/46/commits/76280ebc587fc257758d16067dbe3afd7d2cc7fd since the former uses the two argument form of MoMA which is only added in the latter.

michaelweylandt commented 5 years ago

This can now be rebased (not merged) against the current master.

michaelweylandt commented 5 years ago

One thought re:testing (e.g., https://github.com/michaelweylandt/MoMA/pull/46/commits/31ef3c1ff61ec19cf72b278192e541d620b47c6a): in addition to testing that you get the same results from an R and C++ implementation, you can also test that the theoretical properties hold: e.g., if doing Hotelling deflation with regularization, we would expect u^T X_defl v = 0, but not u^T X_defl = 0, while both should be zero for Schur and projection deflation.

It's not the strictest test in the world, but it's at least a different thing to test which is often a good complement to just checking that you coded the same thing in two languages.

Banana1530 commented 5 years ago

Good idea. Added.

michaelweylandt commented 5 years ago

Great - just an FYI: @agenevera and I are working on systemitizing the deflation / orthogonality related stuff, so let's focus on the other two open PRs for the next few days. I'm gonna try to write up our thoughts before I go out of town Saturday, but I'm not sure exactly when that will be done by.

Banana1530 commented 5 years ago

Gotcha.

michaelweylandt commented 5 years ago
michaelweylandt commented 5 years ago

Can you pull the Shiny stuff out of this PR and just leave it as CCA and LDA? That might help the coverage numbers.

Also, can you clean up the history? It should be possible to move the typo fixes into the original commits for a shorter history.

Banana1530 commented 5 years ago

It is moved to PR #54.