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

Integrate fits of new potentials #57

Merged
merged 67 commits into from
May 30, 2024
Merged

Integrate fits of new potentials #57

merged 67 commits into from
May 30, 2024

Conversation

JaGeo
Copy link
Collaborator

@JaGeo JaGeo commented May 24, 2024

To reactivate the tests, I moved the code into another branch! This will not erase any contributions, just reactivate the tests.

FYI: @QuantumChemist @YuanbinLiu @naik-aakash @vlderinger

Todo:

  • Activate all tests and make them pass on the CI on github
  • Add new installation instructions to Readme

@JaGeo
Copy link
Collaborator Author

JaGeo commented May 24, 2024

@QuantumChemist thank you so much !!!

@JaGeo
Copy link
Collaborator Author

JaGeo commented May 24, 2024

@YuanbinLiu the next step would be activating all fitting tests and getting them to pass!

@@ -1488,16 +1487,16 @@ def energy_remain(in_file):
"""
# read files
in_atoms = ase.io.read(in_file, ":")
if "data_type" in in_atoms[0].info:
if "config_type" in in_atoms[0].info:
Copy link
Collaborator

@QuantumChemist QuantumChemist May 28, 2024

Choose a reason for hiding this comment

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

the correct config_type would be "IsolatedAtoms" (I made config and data types available as the (bulk) config types contain the mpid and the data types contains whether the data is phonon or rattled based)

@QuantumChemist
Copy link
Collaborator

@YuanbinLiu , I'm not sure how you run the unit tests on your local machine, but you can run them all with pytest tests/* in the terminal, and you can also use pytest to run single tests in the terminal or IDE, so that you don't have to run all tests every time. There's in general no need to comment out other unit tests.

@YuanbinLiu
Copy link
Collaborator

@YuanbinLiu , I'm not sure how you run the unit tests on your local machine, but you can run them all with pytest tests/* in the terminal, and you can also use pytest to run single tests in the terminal or IDE, so that you don't have to run all tests every time. There's in general no need to comment out other unit tests.

This is what I used for the test.

@naik-aakash
Copy link
Collaborator

naik-aakash commented May 29, 2024

The failing test is coming from "https://github.com/ACEsuit/ACEpotentials.jl/tree/main", trying to add it to git CI.

@JaGeo, do you have any ideas on how to tackle this? As Acefit uses " Julia" language, I am not sure if it is fine to have two different languages in one environment.

Also, for running tests locally, I got to know from @YuanbinLiu, that he has different environments: one for ace_fit and another for Python-based.

Probably we can skip this ace_fit test or move to some separate tests that use julia

@JaGeo
Copy link
Collaborator Author

JaGeo commented May 29, 2024

@naik-aakash I am happy with a second workflow file or similar. Can one install ace-fit via conda?

@JaGeo
Copy link
Collaborator Author

JaGeo commented May 29, 2024

The only option that does not work in my opinion is skipping tests ;).

Of couse, we should also then add installation instructions including the different fitting frameworks.

@naik-aakash
Copy link
Collaborator

naik-aakash commented May 29, 2024

Hi @JaGeo , I have managed to update the GitHub workflow to install Julia and its dependencies. For some reason it still cannot find installed packages of Julia. Seems I am missing something here. Maybe when you have some time, can you please have a look as well. Am still trying to find what is going wrong.

I have also updated the README with the installation instructions, and it seems to run fine for me locally. All tests seem to pass.

@JaGeo
Copy link
Collaborator Author

JaGeo commented May 30, 2024

@naik-aakash if the coverage is fine, I will merge right away.

@JaGeo
Copy link
Collaborator Author

JaGeo commented May 30, 2024

Thanks to @YuanbinLiu @QuantumChemist @naik-aakash and me! I will merge this as soon as the last test passes.

Just as a side note: we need to increase test coverage for some module files. This could be done in a separate PR.

@JaGeo JaGeo merged commit 268a38c into main May 30, 2024
16 checks passed
@JaGeo JaGeo deleted the fitting_potentials branch October 26, 2024 14:34
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.

4 participants