cvxgrp / pymde

Minimum-distortion embedding with PyTorch
https://pymde.org
Apache License 2.0
526 stars 27 forks source link

Use torch when possible for quadratic initialization #55

Closed adamgayoso closed 2 years ago

adamgayoso commented 2 years ago
adamgayoso commented 2 years ago

Thanks @akshayka. The cg quadratic init was never available through preserve_neighbors, so in the latest change there are three inits again

In the latter case, if device=="cuda" it uses the torch version; otherwise, it uses the scipy version.

akshayka commented 2 years ago

The cg quadratic init was never available through preserve_neighbors, so in the latest change there are three inits again

Yes, I'm aware that CG wasn't available through preserve_neighbors. Even still I'd like to keep preserve_neighbors unmodified in this change. In my mind this function is already too complicated, with too many options. That's my fault, since I wrote it.

I'll note that regardless of what initialization method is passed to preserve_neighbors, users can use custom initializations to the embed method (https://pymde.org/api/index.html#pymde.MDE.embed).

adamgayoso commented 2 years ago

Sounds good, should be up to spec now.

adamgayoso commented 2 years ago

quadratic.spectral still has an awkward signature though because (previously) the device parameter only affects the warm start that users can't even access from the function.

adamgayoso commented 2 years ago

Regarding code formatting, do you use black? If not I can run black and add it in one commit

akshayka commented 2 years ago

Thanks! Yes, I use black, with line length set to 80 (black --line-length 80 quadratic.py, for example). Sorry, I haven't added a config file yet.

I also lint with flake8.

adamgayoso commented 2 years ago

FYI running black on the whole directory changed almost every file, black recently has a new first stable release, probably worth creating another PR adding these new black changes, as well as pre-commit. See here for reference:

https://github.com/scverse/scvi-tools/blob/master/.pre-commit-config.yaml

Import sort is another nice one.

akshayka commented 2 years ago

FYI running black on the whole directory changed almost every file, black recently has a new first stable release, probably worth creating another PR adding these new black changes, as well as pre-commit. See here for reference:

Thanks for the heads up! I'll go ahead and create a new PR that runs a stable version of Black, and will look into pre-commit (thanks for the example). I'll ping you here once that's done.

akshayka commented 2 years ago

@adamgayoso I merged https://github.com/cvxgrp/pymde/pull/56, which run black using version 22.1.0. It also fixed some issues with the CI.

Apologies for the delay! It was a busy week. I didn't get a chance to add pre-commit, I will look into that later.

adamgayoso commented 2 years ago

@akshayka it should be good now!

akshayka commented 2 years ago

@akshayka it should be good now!

Thanks! I left a few comments --- a test related to the scipy LOBPCG codepath is failing. That's my fault, since apparently the Y argument (for imposing a constraint) is not supported when n is small.

Sorry for the confusion!

adamgayoso commented 2 years ago

@akshayka I think I got everything you suggested. Would you be able to make any last changes?

I removed the device argument from spectral() as it was highly ambiguous. Now device is inferred from the input tensors.

akshayka commented 2 years ago

Great, thanks so much! Yes, if there's anything else that needs to be changed, I can make those changes myself.