Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Catch variance equal to zero when scaling #177

Merged
merged 7 commits into from
Mar 24, 2024
Merged

Conversation

dramanica
Copy link
Contributor

This addresses #52
There are now checks both in big_scale(), as well as in big_randomSVD() and crossproductSelf() and tcroosprductSelf(). Unit tests have been added to all functions that were affected.

@dramanica
Copy link
Contributor Author

@privefl I went for belt and braces. There is now a check in the scaling function (in case it is also used in another package), but also in the affected functions within bigstatsr. I guess this adds a redundant check if using big_scale() within a function that already has the checks (e.g. big_randomSVD, but the check is relatively light weight, so I don't think it is a problem (but you are free to disagree!)

@privefl
Copy link
Owner

privefl commented Mar 24, 2024

Thank you for your work on this!

I've added a few changes:

  • only test if it is 0 (instead of also negative) because I sometimes use negative scalings
  • test for near 0 instead of exactly 0
  • warn (instead of error) for big_scale() because I sometimes use this for computing the SD
  • make sure big_randomSVD() also errors in parallel

@privefl
Copy link
Owner

privefl commented Mar 24, 2024

Everything seems to work.
Tell me if you're okay with those changes.

@dramanica
Copy link
Contributor Author

Fine with me!

@privefl privefl merged commit 70729e9 into privefl:master Mar 24, 2024
8 checks passed
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.

2 participants