LSSTDESC / rail_sklearn

RAIL algorithms that depend on scikit-learn.
MIT License
1 stars 0 forks source link

add TEENY to distances #5

Closed sschmidt23 closed 1 year ago

sschmidt23 commented 1 year ago

This PR just adds a TEENY number to the distances in k_nearneigh, as when you have an exact match to a spectroscopic point (e.g. if you accidentally run with the same training and test data) you get a distance = 0 and because we have a weight = 1/dist factor you can get infs and/or NaNs. Adding TEENY to the distances makes sure we do not have a divide by zero.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.04% :tada:

Comparison is base (a2f2546) 95.38% compared to head (eb08ead) 95.43%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #5 +/- ## ========================================== + Coverage 95.38% 95.43% +0.04% ========================================== Files 3 3 Lines 195 197 +2 ========================================== + Hits 186 188 +2 Misses 9 9 ``` | [Files Changed](https://app.codecov.io/gh/LSSTDESC/rail_sklearn/pull/5?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=LSSTDESC) | Coverage Δ | | |---|---|---| | [src/rail/estimation/algos/k\_nearneigh.py](https://app.codecov.io/gh/LSSTDESC/rail_sklearn/pull/5?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=LSSTDESC#diff-c3JjL3JhaWwvZXN0aW1hdGlvbi9hbGdvcy9rX25lYXJuZWlnaC5weQ==) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

aimalz commented 1 year ago

Sorry, what I was referring to was just that this PR (like future bugfixes that arise from discoveries along the way to pipeline development/validation) ought to have included the addition of a unit test of the edge that triggered the problem that was fixed. @sschmidt23 Would you mind writing one on the same branch you used for this PR?

sschmidt23 commented 1 year ago

Ok, that makes sense, sorry for misunderstanding, a unit test is a good idea here. I'll add one soon.