search

ACEsuit / mace

MACE - Fast and accurate machine learning interatomic potentials with higher order equivariant message passing.
Other
413 stars 157 forks source link

a couple of weird statements in new multihead fine-tuning interface code #408

Open bernstei opened 2 months ago

bernstei commented 2 months ago

What is line line supposed to be doing? https://github.com/ACEsuit/mace/blob/fb35950bea8d720c2dde923c9b4fbd153d6b92ab/mace/cli/run_train.py#L184

Why are these keys taken from the user's settings, rather than a default (plain?) property name, or just ignored? https://github.com/ACEsuit/mace/blob/fb35950bea8d720c2dde923c9b4fbd153d6b92ab/mace/cli/run_train.py#L265

ilyes319 commented 1 month ago

First statement is for removing duplicate heads, transforming ["pbe_mp", "pbe_mp", "r2scan"] to ["pbe_mp", "r2scan"]. Second line, we should remove.

bernstei commented 1 month ago

First statement is for removing duplicate heads, transforming ["pbe_mp", "pbe_mp", "r2scan"] to ["pbe_mp", "r2scan"].

if we know that heads starts out as a list, list(set(["pbe_mp"] + heads)) is cleaner in my opinion. But if we care about the order, I'm not sure what set() guarantees. But also, if we care about the order, I think something more explicit is better anyway (e.g. ["pbe_mp"] + list(set(heads) - set(["pbe_mp"])). What exactly are the requirements of the result of this statement?

[edit] Or, maybe cleaner to just do

if "pbe_mp" not in heads:
    heads = ["pbe_mp"] + heads

or heads.insert("pbe_mp", 0), if we want to modify the existing object