metatensor / metatrain

Training and evaluating machine learning models for atomistic systems.
https://metatensor.github.io/metatrain/
BSD 3-Clause "New" or "Revised" License
18 stars 6 forks source link

added architecture #374

Closed DavideTisi closed 3 weeks ago

DavideTisi commented 4 weeks ago

fixes #372


📚 Documentation preview 📚: https://metatrain--374.org.readthedocs.build/en/374/

DavideTisi commented 3 weeks ago

Hi @PicoCentauri @frostedoyster @ppegolo,

so the problem of this PR was, as said in #372, that the default_parameters.yaml files couldn't copy and paste to an actual input file becouse they lacked the architecture tag at the beginning. Why was it a problem? becouse in the documentation->available architecture-> hypers they were displayed, and one couldn't just copy and paste in one's input and modify the parameter, but he would have been stopped by some error (saying that the tag model was not a valid key).

So I changed the default_parameters.yaml files, only problem was that the code used these files to read the default_parameters ( incredible, I know), assuming that there was no architecture tag. My solution was modifying the get_default_hypers inside the utils to return return OmegaConf.to_container(default_hypers)["architecture"] which is exactly what it was returning before. so no code using that function breaks.

The code passes all tests. I hope this was the only place where the absence of architecture in the default_parameters was assumed. Maybe @PicoCentauri knows better.

The new docs for the default hypers are shown in the attached file.

I also corrected some typos when I saw them.

Screenshot 2024-11-04 alle 12 48 49
frostedoyster commented 3 weeks ago

So my comment in the issue was correct and this is more of a user experience issue. We could improve the documentation or write a comment at the top of each of these files, but I also see the point for providing the architecture header in the files so the user would easily recognize and copy-paste them.

I would leave the decision to @PicoCentauri who is the designer of this code

DavideTisi commented 3 weeks ago

Yes, I probably did not understand what you meant in the issue and I did not know it was used in the code. Anyway what ever you want to do is not much work

DavideTisi commented 3 weeks ago

well, I have to say I was very good at implementing the suggestions 😂

PicoCentauri commented 3 weeks ago

hahaha