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

Consider combining different "MS2DeepScore 2" models into one class #204

Open
florian-huber opened this issue Feb 20, 2024 · 2 comments
Open

Comments

@florian-huber
Copy link
Contributor

With #201 we have 3 different models for different steps, which can be confusing.
@niekdejonge proposed to consider ways of merging them into one class.

@florian-huber
Copy link
Contributor Author

To make things more user friendly, we could simply merge the EmbeddingEvaluator and the Linear Model into the SiameseSpectralModel.

  • The EmbeddingEvaluator would be part of the encoder, which would allow people to only call the encoder to get embeddings but also embedding evaluations
  • Add the options for both models to be None in which case the respective models will be skipped during inference.
  • We could either try to create one MS2DeepScore class which will output only scores when no EmbeddingEvaluator and LinearModel are present. (Or we keep two separate matchms-compatible classes like it is now. MS2DeepScoreEvaluated would then raise an exception if the two additional models are missing.)

@niekdejonge
Copy link
Collaborator

My first idea was actually to keep the EmbeddingEvaluator and the LinearModel as separate classes, but to save them to the same file. However, I like the idea of having the option to just calculate embeddings and embedding evalutations. Saving and loading them in one file would reduce the risk of accidental mismatching models that are not compatible. But it could also be confusing to have multiple models stored in one file...

To make this possible we would need to save the embedding_evaluator_state_dict and the ms2deepscore_state_dict under separate keys and during loading use the state dict for each. For the EmbeddingEvaluator I would make it only possible to save to an already existing file that already contains the MS2Deepscore model (you need that model anyway).

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

No branches or pull requests

2 participants