Skip to content
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

Add ".extxyz" file suffix #84

Merged
merged 1 commit into from
Feb 19, 2024
Merged

Add ".extxyz" file suffix #84

merged 1 commit into from
Feb 19, 2024

Conversation

PicoCentauri
Copy link
Contributor

@PicoCentauri PicoCentauri commented Feb 19, 2024

As requested by @frostedoyster. .extxyz and .xyz are usually the same and can use the same readers for them.


📚 Documentation preview 📚: https://metatensor-models--84.org.readthedocs.build/en/84/

Copy link
Contributor

@DavideTisi DavideTisi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great I wanted to ask this feature.
is it possible to add a test for this?
even just change the name of one of the dataset we use to .extxyz

@DavideTisi
Copy link
Contributor

great I wanted to ask this feature. is it possible to add a test for this? even just change the name of one of the dataset we use to .extxyz

I you think is not necessary for me it also fine

@PicoCentauri
Copy link
Contributor Author

great I wanted to ask this feature. is it possible to add a test for this? even just change the name of one of the dataset we use to .extxyz

Yes I can do a symbolic link and test this.

@PicoCentauri
Copy link
Contributor Author

I think that should be enough for testing this.

@Luthaf
Copy link
Member

Luthaf commented Feb 19, 2024

Not a comment for this PR, but long term it might make sense to invert the control flow here. Instead of guessing the reader from the file extension, do something like

structures:
    read_from: file.xyz
    reader: ase

or

structures:
    read_from: ase://file.xyz

this way we don't try to use ASE on CP2K xyz files, which are not quite compatible

@PicoCentauri
Copy link
Contributor Author

Hmm yes, even though I like the second API I think we should go for the first one. Also this comes with less string manioulation if the user leaves the reader field empty.

@DavideTisi
Copy link
Contributor

I like the first API more, I think it is easier to read.
So I agree with going with the first one

@PicoCentauri
Copy link
Contributor Author

Okay I do an issue for this. Then we can keep track.

@PicoCentauri PicoCentauri merged commit aedbf98 into main Feb 19, 2024
8 checks passed
@PicoCentauri PicoCentauri deleted the extxyz branch February 19, 2024 14:41
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