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

Remove ase dependency and update ROY dataset #232

Merged

Conversation

GardevoirX
Copy link
Contributor

@GardevoirX GardevoirX commented Jun 14, 2024

This PR aims at removing the dependency on the ase library. The update of ase leads to issue #231. Since by now only the load of the ROY dataset depends on ase, getting rid of ase is easy and reasonable.

This PR deletes the xyz file and saves the data of the xyz file that is necessary for the example notebook in beran_roy_properties.npz, together with the features originally stored in beran_roy_features.npz. Because of the removal of the xyz file, the visualization of the structures in the example is no longer usable. Perhaps we can consider preserving the xyz file and letting users to open it with ase and then do the visualization.

Contributor (creator of PR) checklist

  • Tests updated (for new features and bugfixes)?
  • Documentation updated (for new features)?
  • Issue referenced (for PRs that solve an issue)?

For Reviewer

  • CHANGELOG updated if important change?

📚 Documentation preview 📚: https://scikit-matter--232.org.readthedocs.build/en/232/

Copy link
Collaborator

@PicoCentauri PicoCentauri left a comment

Choose a reason for hiding this comment

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

Nice! For me this is perfect.

Can you now also remove ase from the dependencies in pyproject.toml

@PicoCentauri
Copy link
Collaborator

I also would like to ask other contributors what they think about removing the roy xyz file.

Especially because the .xyz files is used in for example in https://github.com/lab-cosmo/software-cookbook/blob/chemiscope-output/examples/roy-gch/roy-gch.py

Copy link
Collaborator

@rosecers rosecers left a comment

Choose a reason for hiding this comment

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

This in general looks very good. Future iterations can make use of recent chemiscope developments from @Luthaf and @ceriottm to use the json file type to load the structures, rather than rely on xyz. However, this will lead to a larger file type, so worth discussion in the future.

@PicoCentauri PicoCentauri merged commit f28cfbd into scikit-learn-contrib:main Jun 15, 2024
1 check passed
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