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

Fix tot magnetization base #972

Merged
merged 7 commits into from
Oct 19, 2023

Conversation

AndresOrtegaGuerrero
Copy link
Collaborator

If tot_magnetization is used in overrided , this logic should remove starting_magnetization

@AndresOrtegaGuerrero AndresOrtegaGuerrero marked this pull request as draft October 17, 2023 12:25
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks @AndresOrtegaGuerrero . Added suggestion that I think should fix the failing tests

Comment on lines 201 to 203
# if tot_magnetization in overrides , remove starting_magnetization from parameters
if overrides['pw']['parameters']['SYSTEM'].get('tot_magnetization') is not None:
parameters['SYSTEM'].pop('starting_magnetization', None)
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to guard against SYSTEM not yet being defined.

Suggested change
# if tot_magnetization in overrides , remove starting_magnetization from parameters
if overrides['pw']['parameters']['SYSTEM'].get('tot_magnetization') is not None:
parameters['SYSTEM'].pop('starting_magnetization', None)
# if tot_magnetization in overrides , remove starting_magnetization from parameters
if parameters.get('SYSTEM', {}).get('tot_magnetization') is not None:
parameters.setdefault('SYSTEM', {}).pop('starting_magnetization', None)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sphuber Thank you so much for the help! , I will also continue checking the PdosWorkChain and BandsWorkChain, since somehow it produces a problem of not having a fermi_energy

@sphuber
Copy link
Contributor

sphuber commented Oct 18, 2023

The failure of the docs is unrelated to this PR. I will have to look into this separately to fix it. once that is done, I will rebase and we can merge this

@sphuber sphuber marked this pull request as ready for review October 19, 2023 07:40
@sphuber
Copy link
Contributor

sphuber commented Oct 19, 2023

@AndresOrtegaGuerrero I fixed the docs. Can you please update your branch? Normally I can do it, but for some reason it is not letting me this time

@AndresOrtegaGuerrero
Copy link
Collaborator Author

AndresOrtegaGuerrero commented Oct 19, 2023

@sphuber Thank you ! I just updated, maybe because i am doing the PR from the fork at nanotech-empa ?

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

@sphuber sphuber merged commit 2adf033 into aiidateam:main Oct 19, 2023
6 checks passed
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.

2 participants