Add more loss types and options #385
Conversation
frostedoyster
force-pushed
the
mae-loss
branch
3 times, most recently
from
f0466f9
to
a22914b
Compare
frostedoyster
force-pushed
the
mae-loss
branch
from
a22914b
to
fc6d426
Compare
| and 0.1 for the forces, you can set the following in the ``options.yaml`` file: | ||
| ``loss_weights: {"energy": 1.0, "forces": 0.1}``. | ||
| - ``loss``: This section describes the loss function to be used, and it has three | ||
| subsections. 1. ``weights``. This controls the weighting of different contributions |
There was a problem hiding this comment.
Maybe give the default value here?
| weights: {} | ||
| type: mse |
There was a problem hiding this comment.
I would switch the type and the weight dictionary.
| reduction=self.hypers["loss"]["reduction"], | ||
| type=self.hypers["loss"]["type"], |
There was a problem hiding this comment.
Does it make sens to design the function is such a way that you simply pass
loss_fn = TensorMapDictLoss(**self.hypers["loss"])There was a problem hiding this comment.
Good idea, I'll try to do it
| "oneOf": [ | ||
| { | ||
| "type": "string", | ||
| "enum": ["mse", "mae"] |
There was a problem hiding this comment.
isn't "huber" missing here?
There was a problem hiding this comment.
"huber" is actually an object (not a string) and it's allowed, just a few lines below these
| log_mae: False | ||
| loss: |
There was a problem hiding this comment.
I think we should also just allow
loss: mse
and exand this to
loss:
type: mse
weights: {}
reduction: sum
This should help usability in is in line of what we do for the datasets.
There was a problem hiding this comment.
I think this is confusing for the user, because then the hypers change dynamically. Since we give the user the full template of the hypers to rely on, I would keep the format exactly as in the default hypers template (at least for now)
There was a problem hiding this comment.
I agree with @PicoCentauri here, having a smaller version of the same input is nice, especially if people are not just copy-pasting our templates around. We are already doing this for datasets:
training_set: "dataset.xyz"Actually means
training_set:
systems:
read_from: dataset.xyz
reader: ase
length_unit: null
# ...There was a problem hiding this comment.
Copy-pasting the templates is the only way for users to modify the hypers... they can't make them up. This means that this feature, which is hard to implement, test and maintain, won't be used much
There was a problem hiding this comment.
I'm saying that the overhead for this feature just isn't worth it, considering that the projected gain is from
loss:
type: mae
to
loss: mae
There was a problem hiding this comment.
It's not a lot more, but it is more consistent with other parameters, and nicer to use when writing the input by hand.
I haven't checked the code complexity, but this also looks like a fairly easy thing to implement, so the implementation overhead should be very manageable.
There was a problem hiding this comment.
For now, I think it opens too many possibilities and, from what I can see, it's not easy to test and maintain. Like in programming languages, having many ways of achieving the same thing is not always an advantage. I will open the issue though
There was a problem hiding this comment.
I don't see why the possibilities opened are too many, could you elaborate? Test & maintenance should also be trivial: if the type of loss is a string, you transform to loss: {"type": <value>} and then continue as is. Am I missing something here?
There was a problem hiding this comment.
I could understand if we where trying to be very explicit everywhere, but In general, metatrain options.yaml already includes a lot of syntactic sugar, from default hypers and with the datatset handling; and I really don't see how the loss would be any different.
|
I would merge by the end of today if there are no objections, because we need this feature for some work and the outstanding point is more of a general design issue. I will make sure to open an issue about it |
This PR implements more loss options. Now the user can choose the type of the loss (
rmse,mae,huber) as well as the reduction used (sumormean).Contributor (creator of pull-request) checklist
- [ ] Issue referenced (for PRs that solve an issue)?📚 Documentation preview 📚: https://metatrain--385.org.readthedocs.build/en/385/