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

Use plugin utility to set default starting magnetization #1041

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

edan-bainglass
Copy link
Member

@edan-bainglass edan-bainglass commented Dec 28, 2024

This PR uses the QE plugin get_starting_magenetization utility function to set default initial magnetic moments for the structure when magnetism is turned on. The defaults are set to the default magmom provided by the utility scaled by the Z valence value for the element defined in the selected pseudopotential. Note that for those elements of a zero magmom default, a general default magnetization of 0.1 is returned.

If multiple tags are provided for the same species, a warning is displayed providing some instructions for antiferromagnetic configurations.

Closes #982


Screenshot 2025-01-09 103706

Screenshot 2025-01-09 104547

Screenshot 2025-01-09 104615

@edan-bainglass edan-bainglass self-assigned this Dec 28, 2024
Copy link

codecov bot commented Dec 28, 2024

Codecov Report

Attention: Patch coverage is 78.20513% with 17 lines in your changes missing coverage. Please review.

Project coverage is 67.94%. Comparing base (07593e3) to head (6e05162).

Files with missing lines Patch % Lines
...figuration/advanced/magnetization/magnetization.py 30.76% 9 Missing ⚠️
src/aiidalab_qe/app/configuration/basic/basic.py 25.00% 6 Missing ⚠️
.../app/configuration/advanced/magnetization/model.py 96.77% 1 Missing ⚠️
src/aiidalab_qe/common/mixins.py 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1041      +/-   ##
==========================================
+ Coverage   67.86%   67.94%   +0.07%     
==========================================
  Files         112      112              
  Lines        6679     6726      +47     
==========================================
+ Hits         4533     4570      +37     
- Misses       2146     2156      +10     
Flag Coverage Δ
python-3.11 67.94% <78.20%> (+0.07%) ⬆️
python-3.9 67.96% <78.20%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@edan-bainglass
Copy link
Member Author

@giovannipizzi please confirm the screenshots in the description follow what you had in mind 🙏

@giovannipizzi
Copy link
Member

Looks very good! We can refine in the future if needed, but already now is great

@edan-bainglass
Copy link
Member Author

@AndresOrtegaGuerrero haven't worked much with magnetic calculations. Was wondering how to handle total magnetization now. If starting magnetization is provided, does QE computes total magnetization internally, or do we need to explicitly provided it as well? If so, is it simply the sum of the starting magnetic moments? Note that at the moment, we don't have this implemented in the app. Total magnetization does not observe starting magnetic moments.

@AndresOrtegaGuerrero
Copy link
Member

In the starting_magnetization widget, we're actually passing the initial_magnetic_moments. (Perhaps we should consider renaming the widget for clarity.) These values are then passed to the workchain in the qe-plugin, which generates the starting_magnetization based on the provided magnetic moments.

builder = PwBaseWorkChain.get_builder_from_protocol(
    structure=new_structure,
    code=pw_code,
    protocol='fast',
    spin_type=SpinType.COLLINEAR,
    initial_magnetic_moments=results['magnetic_moments']
)

The values you set in the starting_magnetization widget are treated as magnetic_moments, which are then used by the get_starting_magnetization function to calculate the starting_magnetization values in the input. You can verify this behavior with a test.

It’s important to understand the distinction between starting_magnetization and total_magnetization.

starting_magnetization: Defined per atomic kind, QE will calculate and output the final magnetization after convergence.
tot_magnetization: Specifies the imposed difference between spin-up and spin-down electrons.

For more details, refer to the QE documentation: Quantum ESPRESSO Input Description.

@AndresOrtegaGuerrero
Copy link
Member

One possible solution for #982 could be to screen the dictionary from the starting_magnetization widget. If all values are 0.0, we should pass None. This would allow the PwBaseWorkChain to compute the starting_magnetization values (though this should be tested). If any value is different from 0.0, we pass the full dictionary instead. In this case, renaming the widget to initial_magnetic_moment might make the purpose clearer.

@edan-bainglass
Copy link
Member Author

@AndresOrtegaGuerrero will you be available on Monday to discuss? I have many questions!

@AndresOrtegaGuerrero
Copy link
Member

AndresOrtegaGuerrero commented Dec 29, 2024

So we should rename the widget to magnetic moment, since that is the information we give the PwBaseWorkChain from Qe-plugin.

I think we should have a text below the magnetization title that helps the user.
When magnetic_moment is selected:

If a nonzero ground-state magnetization is expected, you must assign a nonzero value to at least one atomic type. For an antiferromagnetic state, define two distinct atomic species representing the sublattices of the same atomic type.

When Tot_magnetization is selected the help text should say:

The difference between majority spin charge and minority spin charge. Used to specify the desired total electronic magnetization.

@edan-bainglass
Copy link
Member Author

edan-bainglass commented Dec 30, 2024

So we should rename the widget to magnetic moment, since that is the information we give the PwBaseWorkChain from Qe-plugin.

I think we should have a text below the magnetization title that helps the user. When magnetic_moment is selected:

If a nonzero ground-state magnetization is expected, you must assign a nonzero value to at least one atomic type. For an antiferromagnetic state, define two distinct atomic species representing the sublattices of the same atomic type.

When Tot_magnetization is selected the help text should say:

The difference between majority spin charge and minority spin charge. Used to specify the desired total electronic magnetization.

Thanks @AndresOrtegaGuerrero! I'm okay with the text. I would maybe add a line emphasizing that the moments are initial, to be relaxed by the calculation. Maybe also @giovannipizzi can review?

Regarding renaming of the widget, one question I had is how to handle remaining references to starting_magnetization. We use it in references to the parameters dictionary throughout. I could change this, but I'm concerned at its affect on backwards compatibility. Not sure you'd be able to load older calculations. Need to test. That said, the text of the toggle button is already changed to "Magnetic moments", as this does not impact the code.

@AndresOrtegaGuerrero
Copy link
Member

The units of the widget are Bohr magneton (meaning the number of unpaired electrons). QE plugin takes care of converting this value in the units expected in pw.x input (namely starting_magnetization) a value between -1 and 1 (in pw.x not in the App) where 1 means that all electrons available for the selected species will be alpha electron.

Si - Starting magnetization widget = 1

pw.x input
starting_magnetization(1) = 2.5000000000d-01

@edan-bainglass
Copy link
Member Author

edan-bainglass commented Jan 9, 2025

@AndresOrtegaGuerrero @cpignedoli in this PR, I'm using the plugin's get_starting_magnetization utility function

def get_starting_magnetization(
    structure: StructureData,
    pseudo_family: PseudoPotentialFamily,
    initial_magnetic_moments: Optional[dict] = None
) -> dict:
    """Return the dictionary with starting magnetization for each kind in the structure.

    :param structure: the structure.
    :param pseudo_family: pseudopotential family.
    :param initial_magnetic_moments: dictionary mapping each kind in the structure to its magnetic moment.
    :returns: dictionary of starting magnetizations.
    """
    if initial_magnetic_moments is not None:

        nkinds = len(structure.kinds)

        if sorted(initial_magnetic_moments.keys()) != sorted(structure.get_kind_names()):
            raise ValueError(f'`initial_magnetic_moments` needs one value for each of the {nkinds} kinds.')

        return {
            kind.name: initial_magnetic_moments[kind.name] / pseudo_family.get_pseudo(element=kind.symbol).z_valence
            for kind in structure.kinds
        }

    magnetic_parameters = get_magnetization_parameters()
    starting_magnetization = {}

    for kind in structure.kinds:
        magnetic_moment = magnetic_parameters[kind.symbol]['magmom']

        if magnetic_moment == 0:
            magnetization = magnetic_parameters['default_magnetization']
        else:
            z_valence = pseudo_family.get_pseudo(element=kind.symbol).z_valence
            magnetization = magnetic_moment / float(z_valence)

        starting_magnetization[kind.name] = magnetization

    return starting_magnetization

to obtain the starting magnetization, then scale it by the valence provided in the selected pseudo to obtain the moments. Now, Carlo was wondering why 0.4 for Si. Note that get_starting_magnetization obtains the magmom for a given species using the plugin's get_magnetization_parameters utility

def get_magnetization_parameters() -> dict:
    """Return the mapping of suggested initial magnetic moments for each element.

    :returns: the magnetization parameters.
    """
    with (pathlib.Path(__file__).resolve().parent / 'magnetization.yaml').open() as handle:
        return yaml.safe_load(handle)

In "magnetization.yaml", Si's magmom is indeed 0. However, get_starting_magnetization assigns a general default_magnetization: 0.1 (also in the yaml file) when the obtained magmom is 0. The valence of Si is 4, hence 0.4.

Is this correct logic?

@edan-bainglass edan-bainglass marked this pull request as ready for review January 9, 2025 09:00
@cpignedoli
Copy link
Member

Thanks a lot, @edan-bainglass for digging this out. From my use cases this is not ideal. If I am computing Si and for some reason I switch on magnetism I still want to have 0 as suggested starting magnetization.

Moreover, 0.1 converted to 0.4 is additionally not clear to me:
If we consider Fe, the yaml file has magmom = 5, which means 5 electrons with spin up.
Thus in the QE app widget (Bohr magnetons as units) we should have 5 and this should appear in pw.inp as 5/(number of electrons available in the pseudopotential)

So in the case of Si or C, if I set 1 Bohr magneton in the QE app GUI I should see 0.25 in pw.inp

@edan-bainglass
Copy link
Member Author

Thanks a lot, @edan-bainglass for digging this out. From my use cases this is not ideal. If I am computing Si and for some reason I switch on magnetism I still want to have 0 as suggested starting magnetization.

This is a bit difficult to do. I could apply logic to intercept magnetization of 0.1 and zero them out, but that would be assuming that no magmom divided by its pseudo-dependent Z valence value would somehow also hit 0.1, ending up with zero erroneously! Need to think about this some more.

Moreover, 0.1 converted to 0.4 is additionally not clear to me: If we consider Fe, the yaml file has magmom = 5, which means 5 electrons with spin up. Thus in the QE app widget (Bohr magnetons as units) we should have 5 and this should appear in pw.inp as 5/(number of electrons available in the pseudopotential)

So in the case of Si or C, if I set 1 Bohr magneton in the QE app GUI I should see 0.25 in pw.inp

This is all correct and currently implemented. If magmom = 5 in the yaml file, get_starting_magnetization will return the scaled value. I "de-scale" it in the frontend, thus re-obtaining 5. When this is submitted, 5 will again go through that logic and will be rescaled to the correct magnetization. Bottom line, Si magmom of 1 in the app = Si 0.25 magnetization in the PW input.

@edan-bainglass
Copy link
Member Author

edan-bainglass commented Jan 9, 2025

Thanks a lot, @edan-bainglass for digging this out. From my use cases this is not ideal. If I am computing Si and for some reason I switch on magnetism I still want to have 0 as suggested starting magnetization.

This is a bit difficult to do. I could apply logic to intercept magnetization of 0.1 and zero them out, but that would be assuming that no magmom divided by its pseudo-dependent Z valence value would somehow also hit 0.1, ending up with zero erroneously! Need to think about this some more.

Okay, I can solve this. Give me a few minutes 🙏

@edan-bainglass
Copy link
Member Author

Thanks a lot, @edan-bainglass for digging this out. From my use cases this is not ideal. If I am computing Si and for some reason I switch on magnetism I still want to have 0 as suggested starting magnetization.

This is a bit difficult to do. I could apply logic to intercept magnetization of 0.1 and zero them out, but that would be assuming that no magmom divided by its pseudo-dependent Z valence value would somehow also hit 0.1, ending up with zero erroneously! Need to think about this some more.

Okay, I can solve this. Give me a few minutes 🙏

Screenshot 2025-01-09 121643

Automatic defaults 👍 Note the correct 5 for Co and 0s for the rest. @cpignedoli expected?

@edan-bainglass
Copy link
Member Author

Screenshot 2025-01-09 121643

Automatic defaults 👍 Note the correct 5 for Co and 0s for the rest. @cpignedoli expected?

But this I think leads to the same issue, no? If I choose bulk silicon and select magnetism, this "solution" will zero out silicon, thus leading to a non-magnetic calculation! @giovannipizzi @cpignedoli what should we do here? This solution is correct perhaps for the expert, but for beginners, this might be confusing.

@cpignedoli
Copy link
Member

Thanks a lot @edan-bainglass This reflects what I have in mind. We should warn the user that
starting magnetizations are set to ... (which convention has ben used the yaml file?) and the user should check the value and adapt to the know-how related to the specific system under investigation

@giovannipizzi
Copy link
Member

In the function that generates the initial moments, we indeed put 0.1 to ensure we break the "symmetry" and possibly detect any magnetism. If none should be there, the scf should bring it back to zero. This was discussed for quite some time, pinging @mbercx but also @Minotakm @t-reents

For this release, if @cpignedoli is OK, I'd keep the logic in the function (putting some small nonzero also for Si) and we rediscuss. @edan-bainglass this might need a slight change of the explanatory sentence discussed yesterday.

Also as a note, we had a report that setting too high initial moments might make convergence harder and we'll investigate this in the future, so better to discuss this as part of aiida-qe to avoid having two different approaches

@edan-bainglass
Copy link
Member Author

edan-bainglass commented Jan 9, 2025

Thanks a lot @edan-bainglass This reflects what I have in mind. We should warn the user that starting magnetizations are set to ... (which convention has ben used the yaml file?) and the user should check the value and adapt to the know-how related to the specific system under investigation

To be clear, a warning is already in place, pushing users to set the magnetic configuration in the advanced settings. Are you suggesting something in addition to this warning? Note also that once they arrive at the widget in the advanced settings, a help message does instruct the user to set nonzero values to reach a nonzero ground-state magnetization. So I think we're covered?

@cpignedoli
Copy link
Member

sorry @edan-bainglass @giovannipizzi I was not aware of the previous discussions, if the 0.1 was put in place for a good reason, then let's keep it together with the appropriate warnings, I am fine with it. What is important is the consistency with units, the .yaml has the same units as we have in the QE app GUI so we are OK.

@edan-bainglass
Copy link
Member Author

sorry @edan-bainglass @giovannipizzi I was not aware of the previous discussions, if the 0.1 was put in place for a good reason, then let's keep it together with the appropriate warnings, I am fine with it. What is important is the consistency with units, the .yaml has the same units as we have in the QE app GUI so we are OK.

Yes, as long as I apply the rescaling in general, the units are correct. Note that this means for those species of zero magmom, 0.1 magnetization will be rescaled into magmom depending on the Z valence of the element defined in the pseudo. This is how we end up with magmom 0.4 for Si. But it will return back to 0.1 magnetization in the PW input file. As long as we are okay with this, I can implement.

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.

Set default starting magnetization or move to basic settings
4 participants