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

added architecture #374

Merged
merged 6 commits into from
Nov 5, 2024
Merged

added architecture #374

merged 6 commits into from
Nov 5, 2024

Conversation

DavideTisi
Copy link
Contributor

@DavideTisi DavideTisi commented Nov 1, 2024

fixes #372


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

@DavideTisi DavideTisi marked this pull request as ready for review November 4, 2024 11:48
@DavideTisi
Copy link
Contributor Author

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
Copy link
Collaborator

frostedoyster commented Nov 5, 2024

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
Copy link
Contributor Author

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

Copy link
Contributor

@PicoCentauri PicoCentauri left a comment

Choose a reason for hiding this comment

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

It's okay for me. This helps the usability and is just a minor change in one function.

"""
check_architecture_name(name)
default_hypers = OmegaConf.load(get_architecture_path(name) / "default-hypers.yaml")
return OmegaConf.to_container(default_hypers)
return OmegaConf.to_container(default_hypers)["architecture"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I would maybe add a comment here that for a better UX we store the default-hypers.yaml of each architecture indented by one level (architecture:).

USE_SHIFT_AGNOSTIC_LOSS: False # only used when fitting general target. Primary use case: EDOS
ENERGIES_LOSS: per_structure # per_structure or per_atom
CHECKPOINT_INTERVAL: 100
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a new line just for my personal linter peace

@DavideTisi
Copy link
Contributor Author

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

@PicoCentauri
Copy link
Contributor

hahaha

@PicoCentauri PicoCentauri merged commit 71e953b into main Nov 5, 2024
12 checks passed
@PicoCentauri PicoCentauri deleted the parameters-models branch November 5, 2024 15:49
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.

GAP default_hypers are outdated
3 participants