search

lamalab-org / xtal2txt

MIT License
6 stars 0 forks source link

feat: some cleanup #17

Closed n0w0f closed 6 months ago

n0w0f commented 7 months ago

@RezaAliakbari I have done some cleanup. maybe you can review the code?

kjappelbaum commented 6 months ago

@n0w0f and @RezaAliakbari at least for me the tests failed. Probably due to a different Python version. For this reason, I now used PDM to also create lock files.

I also linted the package and removed other unneeded parts (I think our cookiecutter should be updated).

kjappelbaum commented 6 months ago

It would be good to understand the test failures and if we need some additional version pinning or processing of the data to ensure that we don't fool ourselves when we generated millions of text representations (this would especially hurt us, if we collaborate with different python environments)

n0w0f commented 6 months ago

It would be good to understand the test failures and if we need some additional version pinning or processing of the data to ensure that we don't fool ourselves when we generated millions of text representations (this would especially hurt us, if we collaborate with different python environments)

Thank you for reviewing the code, there were some bugs which i have fixed now. Will add more test cases as well.

kjappelbaum commented 6 months ago

@n0w0f the robocrys thing is still failing. Do we need to have a harder pin on the version? Or can we make the test a bit less strict?

n0w0f commented 6 months ago

@n0w0f the robocrys thing is still failing. Do we need to have a harder pin on the version? Or can we make the test a bit less strict?

Not sure about the origin of this issue. Test passed for N2, locally (will cross check dependency local and remote ), and how representation changes with versions. pymatgen version in CI and local is same. for now testing on inorganic system.

n0w0f commented 6 months ago

incorporate #14 #7