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

[BUG] MultipleASE and OmnibusEmbed mistakenly have a .transform method #797

Open
bdpedigo opened this issue Jun 9, 2021 · 4 comments
Open
Labels
bug Something isn't working
Projects

Comments

@bdpedigo
Copy link
Collaborator

bdpedigo commented Jun 9, 2021

Expected Behavior

We never tested or intended the out of sample transform to work for those multiple graph embeddings as the inputs wouldn't even make sense

Additional Details

  • Either of these classes could have a transform method if someone wants to implement it and go through the math
  • For now, I recommend that we just override this method in the BaseEmbedMulti class and have them raise a not implemented error
@bdpedigo bdpedigo added the bug Something isn't working label Jun 9, 2021
@rajpratyush
Copy link

@bdpedigo can i take up this issue??

@bdpedigo
Copy link
Collaborator Author

bdpedigo commented Sep 8, 2021

@rajpratyush sure

@loftusa
Copy link
Collaborator

loftusa commented Nov 16, 2021

@bdpedigo

Are we sure we want to override transform? That prevents us from calling fit then transform separately on our graphs. Usually you wouldn't want to in practice, but I don't see why we should prevent it (since sklearn API allows for it)?

Might be better to check specifically if the new transform inputs are out-of-sample, and then raise NotImplementedError for that. But that is also trickier to implement.

@bdpedigo
Copy link
Collaborator Author

@hugwuoke possible thing to work on

@bdpedigo bdpedigo removed the good first issue Good for newcomers label May 13, 2024
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
bdpedigo
Code quality
Development

Successfully merging a pull request may close this issue.

3 participants