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

Use only lower half of the kernel matrices #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

hclimente
Copy link
Collaborator

Hi @myamada0321,

As we discussed, I implement in this patch a speed-up for HSIC Lasso. When we vectorized the kernel matrix, I use only the lower triangle (diagonal included). Hence, this implementation uses half the memory. LARS might be faster as well.

I had to adapt some tests as well. It seems this causes the reordering of some features, but roughly the same features are selected.

Let me know if you find any issues with this version.

Cheers,
H.

uses less memory, likely speeds up lasso as well
@hclimente
Copy link
Collaborator Author

I am not sure if this is important. But I observed that in the tests, there is one specific feature (299) that always disappears in the tests. I.e. it was selected before, but not anymore. Not sure if that reveals some unexpected behaviour, I will look into it.

@myamada0321
Copy link
Collaborator

Thank you for the update! Reducing to half memory is nice.

I had to adapt some tests as well. It seems this causes the reordering of some features, but roughly the same features are selected.

In general, the results must be identical. Could you check the estimated parameter?

@hclimente
Copy link
Collaborator Author

Indeed, it seems the estimated parameters are not the same. For a toy example:

  • Old run:

Screenshot 2021-07-14 at 12 35 39

  • New run:

Screenshot 2021-07-14 at 12 36 16

I'll see if I can fix this eventually.

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