Function smact_validity slow

[PhilippHoellmer]

The smact_validity function in smact.screening ends with a loop over the combinations of all possible oxidation states. Naturally, depending on the given composition, this loop can take a long time.

From looking at the code, it seems like the loop will always run to the end:

def smact_validity(
    composition: pymatgen.core.Composition | str,
    use_pauling_test: bool = True,
    include_alloys: bool = True,
    oxidation_states_set: str = "icsd24",
) -> bool:
    ...
    compositions = []
    for ox_states in itertools.product(*ox_combos):
        stoichs = [(c,) for c in count]
        # Test for charge balance
        cn_e, cn_r = smact.neutral_ratios(ox_states, stoichs=stoichs, threshold=threshold)
        # Electronegativity test
        if cn_e:
            if use_pauling_test:
                try:
                    electroneg_OK = pauling_test(ox_states, electronegs)
                except TypeError:
                    # if no electronegativity data, assume it is okay
                    electroneg_OK = True
            else:
                electroneg_OK = True
            if electroneg_OK:
                for ratio in cn_r:
                    compositions.append((elem_symbols, ox_states, ratio))
    compositions = [(i[0], i[2]) for i in compositions]
    compositions = list(set(compositions))
    return len(compositions) > 0

I am wondering why this loop isn't broken directly when the first combination of oxidation states is found that passes both the charge-balance test and the electronegativity test? Why is the list compositions even constructed when, in the end, it only matters if a single element is found?

I think the following end of the function should work as well and it has at least the possibility to cut the loop short:

def smact_validity(
    composition: pymatgen.core.Composition | str,
    use_pauling_test: bool = True,
    include_alloys: bool = True,
    oxidation_states_set: str = "icsd24",
) -> bool:
    ...
    compositions = []
    for ox_states in itertools.product(*ox_combos):
        stoichs = [(c,) for c in count]
        # Test for charge balance
        cn_e, cn_r = smact.neutral_ratios(ox_states, stoichs=stoichs, threshold=threshold)
        # Electronegativity test
        if cn_e:
            if use_pauling_test:
                try:
                    electroneg_OK = pauling_test(ox_states, electronegs)
                except TypeError:
                    # if no electronegativity data, assume it is okay
                    electroneg_OK = True
            else:
                electroneg_OK = True
            if electroneg_OK:
                return True
    return False

[aronwalsh]

Thanks for raising this @PhilippHoellmer. That makes sense to me and is an easy win to make things faster.

While the full composition list is useful in some contexts, I don't think it's necessary for this convenience function, especially as it's not even returned. Do you agree @AntObi?

[AntObi]

Yep, that all sounds good! Thanks for pointing that out @PhilippHoellmer! I should have some capacity to get this implemented this week!

[PhilippHoellmer]

Thanks for the updates, @aronwalsh and @AntObi!

[ryannduma]

@AntObi let me know if I can help on this, I can quickly draft a fix on Thursday since I'm working with the smact_validity function at the moment in my latest smact project!