libAtoms / ExtXYZ.jl

Extended XYZ read/write support for Julia
MIT License
13 stars 6 forks source link

Experimental AtomsBase interface #15

Closed jameskermode closed 2 years ago

jameskermode commented 2 years ago

Added an experimental interface to AtomsBase, along with a new concrete type Atoms which is a subtype of AtomBase.AbstractSystem.

JanHab commented 2 years ago

Hello, currently I am working on an University Project which combines Chemfiles.jl, ExtXYZ.jl and pythons ase to simple load and save functions. This project is private yet. We intended to create AtomsBase atomic_systems in load functions and in save functions it should be possible to give an AtomsBase system to the save functions which then saves this system using one of the 3 packages. This would be similar to the way it is done in https://github.com/mfherbst/AtomIO.jl .

Your Experimental AtomsBase interface seems to be great and would make things much easier for us. It would be possible to just use your load function instead of creating an extra AtomsBase system.

I am just wondering why you aren't using any Units? E.g. i think that in AtomsBase it is needed to instantiate the bounding box and the position of the Atoms with a unit.

We were thinking about trying to implement an AtomsBase interface directly in Chemfiles and ExtXYZ, which you have done already. We need to finish our project in the next 2-3 weeks and would like to use your AtomsBase interface, which would make things much easier for us. So I would like to ask you if you intend to accept this pull request or if you wish to change a few things?

jameskermode commented 2 years ago

Thanks for getting in touch - this has only be paused due to lack of time, not because I have plans to change things.I was thinking that the AtomsBase interface didn't really belong in this repo and should instead be a separate package. What are your thoughts on that? I'd also be happy to add units: for ExtXYZ these would be Angstrom for both bounding box and atomic positions. Would you be interested/willing to collaborate on a new package based on the AtomsBase interface defined in this PR and also unit support? Any thoughts on a good name for that package?

JanHab commented 2 years ago

I would be happy to collaborate on this. A separate package would be a good way to to it. I am not sure about the name but maybe something like AtomsBaseExtXYZ?

jameskermode commented 2 years ago

OK great, I'll make a new package+repo and move the code in here there.

The name needs to be more flexible than AtomsBase and more general than ExtXYZ. How about plain Atoms.jl? There was a previous package with that name, but it's no longer in use. Might fall foul of the Julia package naming conventions, though.

@mfherbst or @cortner any suggestions?

mfherbst commented 2 years ago

Hi @jameskermode

Thanks for getting in touch - this has only be paused due to lack of time, not because I have plans to change things.I was thinking that the AtomsBase interface didn't really belong in this repo and should instead be a separate package.

Well depends how you think of it.

(1) In some sense the hope of AtomsBase is that whatever atomic structure you have in julia, you can directly plug it into a follow-up library. A bit like the AbstractArray interface. You don't need to think which package an array type came from, you can just call sum on it or whatever and it works. In that sense ExtXYZ should directly provide the interface and it should be implemented here.

(2) On the other hand if you think of ExtXYZ just as a thin parser which does do as little beyond that as possible, than probably it should not live here. In that case I would actually vouch for having the code directly in AtomsIO (which is the package @JanHab is currently working on with a few others), where the idea is exactly to be a generic library to provide the right parsing and return the data as AtomsBase-compatible data structures.

My feeling is that (1) is the better option for the ecosystem, but that (2) could be easier to achieve (e.g. question on units, question on names of functions, etc.). So if you @jameskermode don't see a way for (1) to be easily done, I'd say we nick your code and put it in AtomsIO if you don't mind.

Maybe I should add: The timescale for AtomsIO to be come public is within the next months as the project of @JanHab and others is coming to a close soon.

mfherbst commented 2 years ago

Oh and I should add that in case (1) needs modifications in AtomsBase that it would be well worth discussing them, naturally.

jameskermode commented 2 years ago

OK, I'm happy to take path (1) you suggest above, particularly since it's less work! I'll merge this PR now and tag a release, then please open Issues here if there are further changes needed.

No modifications required in AtomsBase yet unless the flexible-but-fast Atoms type proves sufficiently useful that others would like to use it without needing to depend on ExtXYZ and it's dependent C code, which is something we can return to run the future.

mfherbst commented 2 years ago

Cool! Thanks for the merge!

jameskermode commented 2 years ago

v0.1.6 now released in General registry

jameskermode commented 2 years ago

@JanHab please do open a PR to fix the units issues you mentioned