search

libAtoms / ExtXYZ.jl

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

Make compatible with AtomsBase 0.4.x #51

Closed cortner closed 1 month ago

cortner commented 2 months ago

The interface in AB0.4 changed slightly. This PR is to update. I don't plan to support both AB 0.3 and 0.4, so this would become a breaking change to be registered as ExtXYZ@0.2.

NB - tests still fail, I will ping once they pass.

jameskermode commented 2 months ago

Thanks, I'll take a look. Tests are currently failing so I can't merge as is.

cortner commented 2 months ago

Definitely not ready yet. I'll let you know.

cortner commented 1 month ago

tests pass now, but maybe this needs a brief discussion before merging. I cannot be sure that I've kept the spirit of some design choices. And there are also some arguments I think to simplify ExtXYZ a bit.

The main change in AtomsBase@0.4 that I struggled to incorporate here, is that atomic_number and atomic_symbol is now a property that is derives from species. Here, species(system, i) is the species of particle i. It could be anything, but if it is an atom then atomic_number(system, i) == atomic_number(species(system, i)) and so forth. The recommendation now is that a system stores ONLY the species. But none of the implementations so far actually follow this and that is causing a lot of hacking.

Either now or in the very near future I would prefer if ExtXYZ.Atoms stores the species either as a ChemicalSpecies (the AtomsBase recommendation) or as a Symbol, but not all three (Z, species and atomic_symbol)

Incidentally, this is the main reason I had to rewrite the test for equality. (which I'm afraid is an incomplete test now.)

cortner commented 1 month ago

If you would like to make changes to the PR, please just go ahead. I'm giving you push access to the fork.

cortner commented 1 month ago

I forgot to mention ... there are a few monkey-patches in the test files that I need to remove but I first have to make some bug fixes to AtomsBase.

jameskermode commented 1 month ago

Ok, will wait for those before merging.

I'll take a look at the species vs Z issue in the meantime. My original idea was to allow either or both to be present and to check for inconsistencies, raising an error if so. I'll look if this is still the case.

cortner commented 1 month ago

NOTE : The tests will ~(likely)~ fail now. To pass they require AtomsBase.jl#118 to be merged and registered. Once this is done then the current PR should be ready to merge.

cortner commented 1 month ago

There is also this discussion where it would be useful to get more input.

jameskermode commented 1 month ago

Thanks. Shall I register as 0.2 or do we need to wait for AtomsBase?

cortner commented 1 month ago

I sent the registration request. Sorry I did this via an issue - forgot to add the registrator message to the commit.