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

Change how eval function works #65

Merged
merged 7 commits into from
Feb 9, 2024
Merged

Change how eval function works #65

merged 7 commits into from
Feb 9, 2024

Conversation

PicoCentauri
Copy link
Contributor

@PicoCentauri PicoCentauri commented Feb 7, 2024

the eval function will now take an options.yaml as the train function to evaluate the RMSE if such data is available. The user can also just removes the targets if he doesn't have target data. Then we only predict and write a file without prompting the RMSE values.

the internal functions of eval will now also be used to report the test, train and validation rmse at the end of the training.

TODO

  • finish implementation of actual target evaluation
  • Add documentation
  • Write tests*
  • Especially that the test splits are the same between train and eval.

@Luthaf
Copy link
Member

Luthaf commented Feb 8, 2024

I'm not sure I understand why this is needed? In case we want to eval only energies and not forces?

@PicoCentauri
Copy link
Contributor Author

@frostedoyster do you have an idea why the regression test is failing?

Copy link
Collaborator

@frostedoyster frostedoyster left a comment

Choose a reason for hiding this comment

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

I really don't know if having the same configuration file for training and eval can work. They're too different...


metatensor-models eval model.pt qm9_reduced_100.xyz
metatensor-models eval options.yaml model.pt --eval_on="training"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure about this... should we just allow the user to input a single dataset (or multiple) without specific names? In particular, having a training set makes little sense when you want to evaluate a model. This will be very confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did another comment. We can change this but this means people have to write another yaml file.

from .formatter import CustomHelpFormatter


logger = logging.getLogger(__name__)

CHOICES_EVAL_ON = ["training", "validation", "test"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a big fan. Here IMO the datasets should have custom names given by the user. I might want to evaluate the "universal" PET model on my toy "silicon" dataset and on a "water" dataset, which will be read from files, but training, validation, test don't make much sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point. But then people have to write a new .yaml file? Is this what we want?

@@ -192,70 +178,68 @@ def _train_model_hydra(options: DictConfig) -> None:
)
train_targets = read_targets(train_options["targets"])
train_dataset = Dataset(train_structures, train_targets)
train_size = 1.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this hardcoded now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, if you read further below thsi is adjusted.

docs/src/getting-started/usage.rst Outdated Show resolved Hide resolved
docs/src/getting-started/usage.rst Outdated Show resolved Hide resolved
examples/basic_usage/usage.sh Outdated Show resolved Hide resolved
src/metatensor/models/cli/eval_model.py Outdated Show resolved Hide resolved
src/metatensor/models/cli/eval_model.py Outdated Show resolved Hide resolved
src/metatensor/models/cli/train_model.py Outdated Show resolved Hide resolved
@frostedoyster frostedoyster self-requested a review February 9, 2024 05:29
@@ -66,25 +66,36 @@ The sub-command to evaluate an already trained model is

metatensor-models eval

Besides the trained `model` you will also have to provide a file containing the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Besides the trained `model` you will also have to provide a file containing the
Besides the trained `model`, you will also have to provide a file containing the

src/metatensor/models/cli/train_model.py Show resolved Hide resolved
@frostedoyster frostedoyster merged commit cdb74f9 into main Feb 9, 2024
8 checks passed
@frostedoyster frostedoyster deleted the eval branch February 9, 2024 08:03
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.

3 participants