Skip to content

Add custom as_dict/from_dict method for proper initialization of attributes of IcohpCollection #4391

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

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

Conversation

naik-aakash
Copy link
Contributor

@naik-aakash naik-aakash commented May 6, 2025

Currently IcohpCollection instance is not serialized correctly, thus I added custom from_dict and as_dict methods here.

@naik-aakash naik-aakash marked this pull request as draft May 6, 2025 20:04
@naik-aakash naik-aakash changed the title add custom from_dict method for proper initialization of attributes of IcohpCollection [WIP] add custom from_dict method for proper initialization of attributes of IcohpCollection May 6, 2025
@naik-aakash
Copy link
Contributor Author

Dear @shyuep ,

I wanted to know what the implications would be if one does not use the unitized decorator for the Vasprun.final_energy property and method here

I am asking this because I ran into serialization issues when working on the app in crystal-toolkit (See materialsproject/crystaltoolkit#459).

The issue to be precise orginates from plotly not being able to serialize FloatWithUnit class objects.

@naik-aakash naik-aakash changed the title [WIP] add custom from_dict method for proper initialization of attributes of IcohpCollection Add custom from_dict method for proper initialization of attributes of IcohpCollection May 16, 2025
@naik-aakash naik-aakash marked this pull request as ready for review May 16, 2025 06:48
@JaGeo
Copy link
Member

JaGeo commented May 16, 2025

Hi @shyuep , @mkhorton , @esoteric-ephemera ,

does anyone of you have more insight on this issue? We ran into this when preparing our LOBSTER data for the CrystalToolkit and corresponding visualization.

@esoteric-ephemera
Copy link
Contributor

Not sure I understand: how is Vasprun being serialized by plotly? There's no real deserialization of Vasprun: while it's MSONable, the roundtrip from_dict can't deserialize a Vasprun from its as_dict()

You can also use the as_dict representation where all unitized objects are base numeric types:

Vasprun(...).as_dict()['output']['final_energy']

@naik-aakash
Copy link
Contributor Author

naik-aakash commented May 17, 2025

Not sure I understand: how is Vasprun being serialized by plotly? There's no real deserialization of Vasprun: while it's MSONable, the roundtrip from_dict can't deserialize a Vasprun from its as_dict()

You can also use the as_dict representation where all unitized objects are base numeric types:

Vasprun(...).as_dict()['output']['final_energy']

Hi @esoteric-ephemera, yes, Vasprun cannot be deserialized. For the app that I am working on in the Crystal Toolkit, deserialization of Vasprun is not needed, as I need it only once during initialization.

Maybe I got it wrong before, I think serialization happens via dash, not plotly, I suppose here https://github.com/materialsproject/crystaltoolkit/blob/79e6a9981f0af0a97109e2f7e9343aefc0d75a1c/crystal_toolkit/core/mpcomponent.py#L270,

When I try to initialize the app, I get the error TypeError: Object of type FloatWithUnit is not JSON serializable.

@naik-aakash naik-aakash changed the title Add custom from_dict method for proper initialization of attributes of IcohpCollection Add custom as_dict/from_dict method for proper initialization of attributes of IcohpCollection May 17, 2025
@esoteric-ephemera
Copy link
Contributor

My guess is that something is not going right on the crystaltoolkit side since I'm able to do

json.dumps(Vasprun(...).as_dict())

without issue. Maybe just pass the as_dict of Vasprun as an object to the dash store for now?

@naik-aakash
Copy link
Contributor Author

My guess is that something is not going right on the crystaltoolkit side since I'm able to do

json.dumps(Vasprun(...).as_dict())

without issue. Maybe just pass the as_dict of Vasprun as an object to the dash store for now?

Thanks @esoteric-ephemera , this can work, maybe I will adapt a bit of stuff in our LobsterPy API too then it should work fine without needing any changes.

@naik-aakash
Copy link
Contributor Author

Hi @JaGeo @shyuep , this PR is ready to be reviewed/merged .

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.

3 participants