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

Add embedding evaluation model #201

Merged
merged 38 commits into from
Feb 20, 2024
Merged

Add embedding evaluation model #201

merged 38 commits into from
Feb 20, 2024

Conversation

florian-huber
Copy link
Contributor

@florian-huber florian-huber commented Feb 13, 2024

  • new EmbeddingEvaluatorModel (Inception Time CNN)
  • new LinearModel
  • new MS2DeepScoreEvaluated matchms-style score --> gives "score" and "predicted_absolute_error"
  • linting and minor documentation changes

@florian-huber florian-huber marked this pull request as ready for review February 16, 2024 15:06
Copy link
Collaborator

@niekdejonge niekdejonge left a comment

Choose a reason for hiding this comment

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

Great work Florian! Mostly some comments about adding documentation. And some potential future changes.

ms2deepscore/models/EmbeddingEvaluatorModel.py Outdated Show resolved Hide resolved


class DataGeneratorEmbeddingEvaluation:
"""Generates data for training an embedding evaluation model.
Copy link
Collaborator

Choose a reason for hiding this comment

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

SettingsMS2Deepscore is already stored in ms2ds_model, so we could just retrieve the settings from there, instead of passing it as a separate parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true. But somehow also a weakness of the on-for-all setting object. For us that might be handy, but I could imagine that this hides a bit of what's happening to new users. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, good point, I see how it can be confusing. I do actually not expect many people to dive that deep into the codebase. Some will train new models, but I would expect them to use the wrapper functions that we provide. So to me, the most important people to design for is ourselves in the future. So the design that is clearest to us in 1 or 2 years. Still I do not know which of the two that is... I would use the settings in the model, or I would make a separate evaluatorSettings class. We now save them separately, but still save all the settings of the "other" models in there as well, which is a bit unexpected.

ms2deepscore/train_new_model/data_generators.py Outdated Show resolved Hide resolved
ms2deepscore/models/EmbeddingEvaluatorModel.py Outdated Show resolved Hide resolved
ms2deepscore/models/EmbeddingEvaluatorModel.py Outdated Show resolved Hide resolved
ms2deepscore/models/load_model.py Show resolved Hide resolved
@florian-huber florian-huber merged commit 91e9808 into dev_pytorch Feb 20, 2024
9 checks passed
@florian-huber florian-huber deleted the add_2nd_model branch February 20, 2024 08:24
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