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

Always use periodic_table.csv file embedded in the repo #1191

Merged
merged 4 commits into from
Oct 24, 2023

Conversation

niklassiemer
Copy link
Member

No description provided.

setup.py Outdated
@@ -61,5 +61,5 @@
'structuretoolkit==0.0.11'
],
cmdclass=versioneer.get_cmdclass(),

package_data={'': ['_data/*.csv']},
Copy link
Member

Choose a reason for hiding this comment

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

Is there a specific reason to name the folder _data rather than just data?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I considered this private to the repo. However, since there is no __init__ (at least not yet). It will not pop up in the dir of the package.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I also do not expect there to be any __init__ in the data directory but on the root level of the repository I would prefer to have it as data rather than _data or .data.

@pmrv
Copy link
Contributor

pmrv commented Oct 16, 2023

I think the "proper" way to access package data is via pkgutil, then you can also hard code "pyiron_atomistics" as the package name rather than relying on __file__.

Copy link
Member

@jan-janssen jan-janssen left a comment

Choose a reason for hiding this comment

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

Looks good to me

@niklassiemer niklassiemer merged commit de5068d into main Oct 24, 2023
23 checks passed
@delete-merged-branch delete-merged-branch bot deleted the repo_pse_csv branch October 24, 2023 09:22
@jan-janssen
Copy link
Member

@niklassiemer I guess it makes sense to release a new version of pyiron_atomistics, so we can fix the tests of pyiron_contrib. Can you do this?

@liamhuber liamhuber mentioned this pull request Feb 1, 2024
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.

Atomistics causing downstream tests to fail because periodic table can't be found
3 participants