Repository navigation
feat: structure object to ASE structure objects adapter #127
Description
Activity
I can attempt on this issue. However, this issue may take me a month. Is there a deadline on this issue?
@TingwenZhang I suggest that you work on one or two of the other issues first to practice the workflow, then we can switch to this one (which is the main event and can take longer). we have a particular workflow where we discuss first behavior, then design, then tests then the code.
Got it.
Reacted by Simon BillingeMoving the discussion on this PR to here. Below is some of that discussion:
from @cadenmyers13
Here's the UCs i thought of and is what guided the code on this PR. Only option 2 is included here. Option 1 would be on a different PR. Both 1a and 1b would be things that would need to be implemented in ase, but i included them here for completion:
User wants to convert diffpy --> ase
a. User calls a method in diffpy.structure.Structure and passes diffpy obj to convert diffpy --> ASE, or
b. User calls a method in ASE.Atoms and passes diffpy obj to convert a diffpy --> ASEUser want to convert ase --> diffpy
a. User calls a method in diffpy.structure.Structure and passes ase obj to convert diffpy --> ASE, or
b. User calls a method in ASE.Atoms and passes ase obj to convert a diffpy --> ASEAny other use cases you could think of?
Also, we might be able to get away with this without adding it as a dependency. The only use for it is an isinstance check that raises an error, but we could probably remove this and the user would get an error downstream generated by something else. Presumably, if you're trying to convert a diffpy object to ase you already have ase installed in your env. Idk how removing this will go over in testing though. We could just add ase to tests.txt, right?
from @sbillinge
The main question is serialization. I think that if it is possible to serialize ASE objects we can read them without an ASE dependency. We can also have code to write them.
Your UCs are good for some kind of high throughout pipeline where ASE is generating large numbers of objects that are being passed to Diffpy and serialization would be a serious performance bottleneck. However I do see the use and convenience. It occured to me that we could make an ASE pack so we don't pick up a new dependency in the core.
I think that thinking of actual UCs could be good. The one highest on my list would be to take an MD simulation and Compute the PDF. Another would be to support something like Soham's cluster mining. We should maybe check how he did it as he may already have some code. I would guess that the code is on gitlab
For these UCs do you know if it is Atoms objects or Some other kind of ASE objects that is passed?
Your UCs are good for some kind of high throughout pipeline where ASE is generating large numbers of objects that are being passed to Diffpy and serialization would be a serious performance bottleneck. However I do see the use and convenience. It occured to me that we could make an ASE pack so we don't pick up a new dependency in the core.
@sbillinge ChatGPT recommends doing a lazy import and not make it a dependency and not to serialize. It says this is common practice. Something like this,
try: from ase import Atoms except ImportError: raise ImportError("ASE is required for conversion to ASE objects.")
For these UCs do you know if it is Atoms objects or Some other kind of ASE objects that is passed?
ase.Atomsis the analogous object todiffpy.structure.Structure.I agree with you about the MD workflow. I don't think that would be too difficult to implement with a lazy import. The UC would be like:
- User run MD
- they get an
ase.Atomsobject out - they want to compute a PDF of it
- they pass the
ase.Atomsobject todiffpy.structure.Structure.convert_ase_to_diffpy() - the analogous
diffpy.structureobject is instantiated with all necessary information. - User can then easily calculate pdf from this object
TLDR: Let's get started on an
asetodiffpy.structureadapter using a lazy import. what do you think?
Problem
we would like adapters between diffpy.structure structure objects and ASE structure objects
Proposed solution