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

Simplify arguments and reintroduce aggregation mode #2

Merged
merged 4 commits into from
Sep 14, 2018

Conversation

ktschuett
Copy link
Contributor

Resolves #1:

  • property argument is only needed during training call
  • old "pool_mode" is reintroduced to cmd args as "aggregation_mode" (more fitting name)

@ktschuett ktschuett requested a review from a team September 12, 2018 08:12
@cod3licious
Copy link

cod3licious commented Sep 12, 2018

I think at least for schnetpack_qm9.py this doesn't fix everything yet. When evaluate() is called, it is given args instead of train_args but uses args.property. However, it can't just be switched like that, as it also needs args.modelpath, which is evaluation specific (should it be? are folder names frequently changed between training and evaluation?).

Also I guess --logger and --log_every_n_epochs are also only relevant for training and not as general arguments.

Maybe in general at the start of the script in eval mode, train_args could be loaded and then only the "active" arguments of the current regular args could be used to replace the corresponding fields in train_args, and then everywhere only use train_args and not this mix between both, which makes it kind of hard to keep track of what goes where?

@ktschuett
Copy link
Contributor Author

Ok, then we need to pass properties separately. I will take a look later and also move the logger and remove the model-parameters from eval since they are fixed with the trained model.

Of course the model path needs to be specific to the evaluation, otherwise, how should we know where the model is stored?

@cod3licious
Copy link

yes, of course you need to pass the modelpath when calling eval, but the path itself should still be the same as when the model was trained. And I think the modelpath and maybe flags like --cuda might be the only thing that really need to be given for eval? The data as well should still be at the same place as when training the model, no?

@ktschuett ktschuett requested review from mgastegger and a team and removed request for mgastegger September 14, 2018 08:25
Copy link
Collaborator

@mgastegger mgastegger left a comment

Choose a reason for hiding this comment

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

Looks fine.

@ktschuett ktschuett merged commit af7b642 into master Sep 14, 2018
@ktschuett ktschuett deleted the simplify_args1 branch September 19, 2018 13:27
epens94 referenced this pull request in epens94/schnetpack Apr 26, 2023
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.

None yet

4 participants