Skip to content

Conversation

@aryakrekhi
Copy link
Contributor

No description provided.

@aryakrekhi aryakrekhi requested a review from aarmey February 23, 2022 19:31
@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Merging #161 (2e98627) into main (348aec9) will decrease coverage by 0.52%.
The diff coverage is 62.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #161      +/-   ##
==========================================
- Coverage   51.28%   50.76%   -0.53%     
==========================================
  Files          16       16              
  Lines         969      979      +10     
==========================================
  Hits          497      497              
- Misses        472      482      +10     
Flag Coverage Δ
unittests 50.76% <62.85%> (-0.53%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
de/linearModel.py 26.66% <14.28%> (-6.67%) ⬇️
de/impute.py 49.01% <40.00%> (-11.92%) ⬇️
de/fancyimpute/soft_impute.py 86.95% <75.00%> (-9.97%) ⬇️
de/factorization.py 100.00% <100.00%> (ø)
de/fancyimpute/solver.py 61.85% <0.00%> (+3.09%) ⬆️

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 348aec9...2e98627. Read the comment docs.

@aarmey
Copy link
Member

aarmey commented Feb 23, 2022

@aryakrekhi @Farnazmdi ok, this now has moved imputation to within the fitting loop. I think this will be much faster and have similar results to before. Please try it out and see if I'm right.

@Farnazmdi
Copy link
Contributor

Thank you! It certainly is faster, however, I ran the figure for comparing linear and nonlinear fitting, they don't seem to be significantly different...
image

@aarmey
Copy link
Member

aarmey commented Feb 24, 2022

Is this a within-row correlation like we talked about? I forget if that's already been merged.

@Farnazmdi
Copy link
Contributor

Yes, it is reflected in line 12 of impute.py file.

@aarmey
Copy link
Member

aarmey commented Feb 24, 2022

How are you running the linear impute?

@Farnazmdi
Copy link
Contributor

Ah.. that's the reason.. I should have changed it based on the new implementation. I think it doesn't actually run the linear at all, both boxplots are from nonlinear..
I will work on this and get back to you.

@Farnazmdi
Copy link
Contributor

Farnazmdi commented Feb 27, 2022

I edited the linear model in the new imputation setting and ran the correlation figure again. The changes are pushed, the figure took 144473 seconds (40 hours) to run -- slow because of linear model!! And the linear model looks better in most of them!

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants