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

Nexus: Element names need to be capitalized in incorporate_system at fileio.py #4958

Merged
merged 5 commits into from
May 10, 2024

Conversation

kayahans
Copy link
Contributor

@kayahans kayahans commented Apr 17, 2024

Proposed changes

Element names need to be capitalized to be able to run qdens.
For a MoS2 calculation, I used the qdens in the following way:
qdens -e 100 -a -v -f xsf -i dmc.g000.twistnum_0.in.xml *.s002.stat.h5

With averaging over multiple twists, the run dies without any error message (only prints "Killed" and terminates).
For a single twist, I get the following error:
*** KeyError: 'MO'

After debugging, I found that MO should stand for Molybdenum. After converting MO to Mo, qdens script works as expected for a single twist.

What type(s) of changes does this code introduce?

  • Bugfix

Does this introduce a breaking change?

  • No

What systems has this change been tested on?

Bilayer MoS2

@prckent
Copy link
Contributor

prckent commented Apr 24, 2024

Can you explain where the uppercase MO is coming from in your example? Ideally Nexus has and enforces a consistent use throughout. The fix here is user friendly so we could run with it, but there must be other cases where capitalization is not either converted or enforced.

@kayahans
Copy link
Contributor Author

I think the bug is because of this:

Nexus makes the upper/lower case correction of qmcpack input by keeping a list of parameters that are used in the XML file. Therefore the Molybdenum atom symbol is converted from 'Mo' to 'MO' in the qmcpack input file, despite it was written as 'Mo' in the nexus input script. I think this is a problem specific to Mo atom, because the "MO" is a tag in qmcpack input. This does not interfere with the qmcpack run, but only seems to be a problem when the structure info is read from the xml file (in qdens), where qmcpack does not check capitalization.

Copy link
Contributor

@prckent prckent left a comment

Choose a reason for hiding this comment

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

@jtkrogel Any potential side effects of this update we should be aware of?

(Given @kayahans 's analysis, I think the current fix seems very reasonable and pragmatic)

@jtkrogel
Copy link
Contributor

jtkrogel commented May 9, 2024

There is already a feature that performs this type of function (atomic symbol processing). Better to use this rather than duplicate.

I think this loop in file fileio.py:

        for e in s.elem:
            ne = len(e)
            if ne>1:
                if ne==2 and not e[1].isalpha():
                    e = e[0]
                elif ne>2:
                    e = e[0:2]
                #end if
            #end if
            if is_element(e):
                elem.append(ptable.elements[e].atomic_number)
            else:
                elem.append(0)
            #end if
        #end for

Can be replaced with this one:

        for e in s.elem:
            is_elem,e = is_element(e,symbol=True)
            if is_elem:
                elem.append(ptable.elements[e].atomic_number)
            else:
                elem.append(0)
            #end if
        #end for

@kayahans If you try this, does it fix the issue?

@kayahans
Copy link
Contributor Author

kayahans commented May 10, 2024

@jtkrogel's suggestion works, pushed a new commit with his suggestion.

Copy link
Contributor

@prckent prckent left a comment

Choose a reason for hiding this comment

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

Thanks all

@prckent
Copy link
Contributor

prckent commented May 10, 2024

Test this please

@prckent prckent changed the title [Nexus] Element names need to be capitalized in incorporate_system at fileio.py Nexus: Element names need to be capitalized in incorporate_system at fileio.py May 10, 2024
@ye-luo
Copy link
Contributor

ye-luo commented May 10, 2024

Test this please

@ye-luo ye-luo enabled auto-merge May 10, 2024 19:42
@ye-luo ye-luo merged commit f2d6438 into QMCPACK:develop May 10, 2024
38 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.

None yet

4 participants