search

meyer-lab / systemsSerology

Dissecting systems serology with a tensor factorization
https://asmlab.org
3 stars 2 forks source link

Fix delete component #266

Closed cyrillustan closed 3 years ago

cyrillustan commented 3 years ago

Don't think this fix the problem entirely though

codecov[bot] commented 3 years ago

Codecov Report

Merging #266 (5113143) into master (3eff47f) will decrease coverage by 1.20%. The diff coverage is 70.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #266      +/-   ##
==========================================
- Coverage   93.76%   92.56%   -1.21%     
==========================================
  Files           7        7              
  Lines         353      363      +10     
==========================================
+ Hits          331      336       +5     
- Misses         22       27       +5
Flag Coverage Δ
unittests 92.56% <70.58%> (-1.21%) :arrow_down:

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

Impacted Files Coverage Δ
syserol/tensor.py 93.06% <70.58%> (-4.74%) :arrow_down:

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 3eff47f...5113143. Read the comment docs.

cyrillustan commented 3 years ago

My guess is after we remove components manually, tl.cp_to_tensor() cannot render the reconstructed data tensor correctly. Or alternatively, the missing values are ignored by np.nanvar(), but the reconstructed data tensor doesn't include any missing value. Let me merge this first, then we can fix this with another PR.

aarmey commented 3 years ago

Factors, weights, rank, and shape are the only variables in the CP tensor object, though:

https://github.com/tensorly/tensorly/blob/main/tensorly/cp_tensor.py#L29

Agreed—merge, then we can think about any unexpected behavior.