-
Notifications
You must be signed in to change notification settings - Fork 4
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
Provide better logging #37
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice job! I have one minor possible improvement.
Question is if you want to test the logging somehow. It could be done and if you like I can show it to you.
for key, value in aggregated_train_info.items(): | ||
if key.endswith("_positions_gradients"): | ||
# check if this is a force | ||
target_name = key[: -len("_positions_gradients")] | ||
if model.capabilities.outputs[target_name].quantity == "energy": | ||
# if this is a force, replace the ugly name with "force" | ||
if only_one_energy: | ||
key = "force" | ||
else: | ||
key = f"force[{target_name}]" | ||
elif key.endswith("_displacement_gradients"): | ||
# check if this is a virial/stress | ||
target_name = key[: -len("_displacement_gradients")] | ||
if model.capabilities.outputs[target_name].quantity == "energy": | ||
# if this is a virial/stress, | ||
# replace the ugly name with "virial/stress" | ||
if only_one_energy: | ||
key = "virial/stress" | ||
else: | ||
key = f"virial/stress[{target_name}]" | ||
logging_string += f", train {key} RMSE: {value:10.4f}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block and the one below seem to me exactly the same. Maybe just do another outer loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good idea
For now, I would merge it. I will open an issue about cleaning up the logging, which will also cover other aspects of the logger |
📚 Documentation preview 📚: https://metatensor-models--37.org.readthedocs.build/en/37/