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

Bugs and performance issues related to PCov-ized sample selection methods #118

Merged
merged 2 commits into from
Oct 22, 2021

Conversation

ceriottm
Copy link
Collaborator

This was started noting a bug in the orthogonalization of targets in PCov-CUR, and evolved to also tackle some performance issues, and a nasty bug where all sample-space PCov selection methods would give nonsense when using a 1D array as the target vector y. Urgent merge.

@ceriottm ceriottm requested a review from rosecers October 22, 2021 07:30
@ceriottm ceriottm added the bug Something isn't working label Oct 22, 2021
Copy link
Collaborator

@Luthaf Luthaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to add some tests that would fail with the old, incorrect behavior but pass with the new one?

@rosecers rosecers force-pushed the bugfix/pcovr_orthogonalize branch 3 times, most recently from 4c8db39 to bf19d9e Compare October 22, 2021 14:48
Loooots of printf debug statements. Also changed regression to lstsq which makes the y orthogonalization 10x faster
@rosecers rosecers force-pushed the bugfix/pcovr_orthogonalize branch from bf19d9e to 740b00e Compare October 22, 2021 14:55
@rosecers rosecers merged commit 8a6a89e into main Oct 22, 2021
@rosecers rosecers deleted the bugfix/pcovr_orthogonalize branch October 22, 2021 14:59
@Luthaf
Copy link
Collaborator

Luthaf commented Oct 25, 2021

Should we do a 0.1.1 release with this bugfix included?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants