Walkthrough

The recent updates across the pymatgen library focus on improving code quality through enhanced readability, efficiency, and robustness. Changes include variable renaming for clarity, refactoring for better logic flow, and the integration of modern Python features like the walrus operator. Additionally, stricter type checking with pyright has been introduced to ensure higher code quality.

Changes

Poem

🐇💻✨
Oh, hop through the fields of code, so spry,
Where bugs once lurked, now none shall lie.
With pyright by side, we leap and bound,
In cleaner pastures, our bytes resound.
Celebrate, dear friends, with vim and cheer,
For cleaner, swifter code is here! 🎉🌟

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Thanks for reviewing @janosh and great to collaborate with you on this one @JaGeo.

Sorry for hijacking this topic @ml-evs.

Sorry for hijacking this topic @ml-evs.

No problem, this is great work! It looks this was fairly comprehensive; I'll try reopening my PR that adds the CI runs that triggered this.

Sorry for hijacking this topic @ml-evs.

No problem, this is great work! It looks this was fairly comprehensive; I'll try reopening my PR that adds the CI runs that triggered this.

This is only for some modules. More work is needed

This is only for some modules. More work is needed

Yes! It's only like 1/3 of the all work needed. I would ping you once it's all ready. @ml-evs

This is only for some modules. More work is needed

Yes! It's only like 1/3 of the all work needed. I would ping you once it's all ready. @ml-evs

Understood, I've opened #3759 with the same changes as before (except enabling pyright), hopefully the CI is now stable enough after the uv switch that it won't go stale again so quickly.

Yes! It's only like 1/3 of the all work needed. I would ping you once it's all ready. @ml-evs

@DanielYang59 thanks a lot for donating your time to work on this! 👍

@DanielYang59 thanks a lot for donating your time to work on this! 👍

Glad I can help :)

@janosh I'm still experimenting on an issue I met with @JaGeo in #3757 (comment). I would appreciate it if you could give me some advice 😄

I just created a minimal code to recreate this issue:

from collections import UserDict

class TestClass(UserDict):

    def __init__(self, settingsdict: dict):
        super().__init__()
        self.update(settingsdict)

    def __setitem__(self, key, val):
        """Remove case sensitivity."""
        new_key = next((key_here for key_here in self if key.strip().lower() == key_here.lower()), key)

        super().__setitem__(new_key, val)

    def __getitem__(self, item):
        """Implements getitem from dict to avoid problems with cases."""
        new_item = next((key_here for key_here in self if item.strip().lower() == key_here.lower()), item)

        return super().__getitem__(new_item)


if __name__ == "__main__":
    test_dict = {"ConTent": "hello", }
    test_instance = TestClass(test_dict)

    print(f"{test_instance=}")
    print(test_instance.get("content"), test_instance["content"])

On this same machine (MacOS Sonoma 14.4.1, Apple Silicon and Python managed by miniconda3).

With Python 3.10 and 3.11, I got:

(base) yang@Yang-MacBook test % python3 -V
Python 3.10.14
(base) yang@Yang-MacBook test % python3 recreate.py 
test_instance={'ConTent': 'hello'}
hello hello

(base) yang@Yang-MacBook test % python3 -V
Python 3.11.8
(base) yang@Yang-MacBook test % python3 recreate.py 
test_instance={'ConTent': 'hello'}
hello hello

While with Python 3.12, the two methods get different results:

(base) yang@Yang-MacBook test % python3 -V               
Python 3.12.2
(base) yang@Yang-MacBook test % python3 recreate.py      
test_instance={'ConTent': 'hello'}
None hello

Is it some known issue/change from Python 3.12?

Ah, interesting, Python version 😅.
This is why it has not shown up on CI.

@DanielYang59 I think I found it: python/cpython#105524

@DanielYang59 I think I found it: python/cpython#105524

👍 So speedy. Yes looks like exactly what we're experiencing. I guess it's not intended?

The get method:

    def get(self, key, default=None):
        if key in self:
            return self[key]
        return default

Which indeed does not use __getitem__.

But at least confirmed my machine is not the "outlier" 😅

@DanielYang59 I think I found it: python/cpython#105524

👍 So speedy. Yes looks like exactly what we're experiencing. I guess it's not intended?

The get method:

    def get(self, key, default=None):
        if key in self:
            return self[key]
        return default

Which indeed does not use __getitem__.

But at least confirmed my machine is not the "outlier" 😅

No idea if intended.
But a solution is described there as well. Could potentially affect other parts of the code as well.

there's only 1 use of UserDict in all of pmg i think. we can try the proposed fix or try to make it work without UserDict. the docs weakly recommend to subclass dict directly instead of UserDict

I think we recently switched to UserDict for other issues.

there's only 1 use of UserDict in all of pmg i think. we can try the proposed fix or try to make it work without UserDict. the docs weakly recommend to subclass dict directly instead of UserDict

I would keep it in mind. I'm now secretly working on simplifying io.lobster.inputs (prefer not to push it here for now because I'm not having much time to work on this).