materialsproject / pymatgen

Python Materials Genomics (pymatgen) is a robust materials analysis code that defines classes for structures and molecules with support for many electronic structure codes. It powers the Materials Project.
https://pymatgen.org
Other
1.52k stars 867 forks source link

[Bug]: oxi_state_guesses does not work for selected diatomic gases #3324

Closed rkingsbury closed 1 year ago

rkingsbury commented 1 year ago

Email (Optional)

No response

Version

latest from git as of 2023.09.14

Which OS(es) are you using?

What happened?

  1. I was using Ion.oxi_state_guesses to determine oxidation states of elements in aqueous species. This method inherits from Composition
  2. No results were given for O2. Subsequent testing revealed that O2,, Cl2, and F2 return empty lists [], whereas N2 and H2 return tuples of dicts, as expected.
  3. I examined Element.common_oxidation_states for each of these elements, but none contains 0 as a common oxi state. Nevertheless, 0 is correctly returned by Composition.oxi_state_guesses for two of them.

Code snippet

>>> Composition('O2').oxi_state_guesses()
[]
>>> Composition('H2').oxi_state_guesses()
({'H': 0.0},)
>>> Composition('N2').oxi_state_guesses()
({'N': 0.0},)
>>> Composition('F2').oxi_state_guesses()
[]
>>> Composition('Cl2').oxi_state_guesses()
[]
>>> Element('O').common_oxidation_states
(-2,)
>>> Element('H').common_oxidation_states
(-1, 1)
>>> Element('N').common_oxidation_states
(-3, 3, 5)
>>> Element('F').common_oxidation_states
(-1,)
>>> Element('Cl').common_oxidation_states
(-1, 1, 3, 5, 7)

Log output

No response

Code of Conduct

janosh commented 1 year ago

Looks like oxidation state of 0 for diatomic molecules isn't specified in Element.icsd_oxidation_states or Element.oxidation_states here

https://github.com/materialsproject/pymatgen/blob/047ce69fe111d53fd6776f8773216bd9a042f987/pymatgen/core/composition.py#L912-L916

Not sure if the better solution is to update that data or just hard-code a special case for single-element diatomic molecules in oxi_state_guesses():

        if len(self.elements) == 1 and self.element_composition[self.elements[0]] == 2:
            return ({self.elements[0].symbol: 0.0},)

Maybe @shyuep or @mkhorton want to weigh in?

rkingsbury commented 1 year ago

I discovered this morning that this can be partially solved by passing all_oxi_states=True, but there still appears to be data missing for F2.

Given how commonly diatomic gases arise in, e.g., formation energy calcs, I also think we should not require a non-default kwarg just to get the correct oxidation state for these substances.

>>> Composition('O2').oxi_state_guesses(all_oxi_states=True)
({'O': 0.0},)
>>> Composition('Cl2').oxi_state_guesses(all_oxi_states=True)
({'Cl': 0.0},)
>>> Composition('F2').oxi_state_guesses(all_oxi_states=True)
[]
>>> Composition('I2').oxi_state_guesses(all_oxi_states=True)
({'I': 0.0},)
>>> Composition('Br2').oxi_state_guesses(all_oxi_states=True)
({'Br': 0.0},)
>>> Composition('N2').oxi_state_guesses(all_oxi_states=True)
({'N': 0.0},)
>>> Composition('H2').oxi_state_guesses(all_oxi_states=True)
({'H': 0.0},)
shyuep commented 1 year ago

I am not actually sure who relies on oxi_state_guesses. As their name implies, they are guesses. The guesses are probably not very useful. For diatomic gases, I would simply set oxidation state to 0 since that's the only possible charge neural solution. In fact, all elemental systems (be it metals, gases or whatever) should have an oxidation state of 0. @janosh Can you correct your recent PR to reflect this?

rkingsbury commented 1 year ago

Thanks for the quick action @janosh and @shyuep 👍🏻