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 short run to git submodule #71

Merged
merged 23 commits into from
Jul 27, 2023
Merged

add short run to git submodule #71

merged 23 commits into from
Jul 27, 2023

Conversation

v1kko
Copy link
Contributor

@v1kko v1kko commented Jul 24, 2023

Depends on #76

Todo

  • Investigate what is going on with e_act
  • Refactor integration test. Can we somehow isolate the group the different stages of the calculations? e.g. test_jumps, test_transitions_matrix, test_all_transition
  • See if it is feasable to test for more 'actionable' numbers (e.g. not sum of matrix)
  • Get rid of environment variable and point to data directly through importlib.resources.files
  • move git lfs to a provider that does not cost $$$ (so not github)
    Use a separate branch instead, which we can later throw away

@v1kko v1kko marked this pull request as ready for review July 24, 2023 10:01
@v1kko v1kko linked an issue Jul 24, 2023 that may be closed by this pull request
@stefsmeets
Copy link
Contributor

stefsmeets commented Jul 24, 2023

Cool, this will be very useful for the integration tests :D

I think we can leave out the OUTCAR file to be honest, all we need is vasprun.xml.

@v1kko
Copy link
Contributor Author

v1kko commented Jul 24, 2023

@stefsmeets I had to update all the reference values in the integration_test.py , but it seems to work now, is it okay that these values are updated?

@stefsmeets
Copy link
Contributor

stefsmeets commented Jul 24, 2023

Yeah, I used them to make sure I didn't randomly break something. Now that we have a smaller data set to work with, we can make better, more isolated integration tests.

Best would be to cross-check with the sim_data.mat / sites.mat to make sure the values we calculate are close. You could also compare the plots once again to see if they are the same.

@v1kko
Copy link
Contributor Author

v1kko commented Jul 24, 2023

Checking against the data / plots is troublesome, well, some plots look similar, but some plots don't, and all the values are different as well 😓 . So I cannot be certain that these changes are correct, but the matlab code run in octave is also very buggy on my end, so perhaps I am not the best person to update these values.
@stefsmeets do you want to take a look?

@stefsmeets stefsmeets self-assigned this Jul 24, 2023
@stefsmeets
Copy link
Contributor

I'm merging #76, because we need the supercell fix to correctly calculate jumps.

@stefsmeets stefsmeets marked this pull request as draft July 25, 2023 13:49
@stefsmeets
Copy link
Contributor

stefsmeets commented Jul 25, 2023

I checked all the numbers and it mostly correlates with the results from the matlab code. I also verified that all the plots look the same / similar.

The only difference we get is in e_act / e_act_std. I will look into what is happening here.

@stefsmeets
Copy link
Contributor

stefsmeets commented Jul 25, 2023

@v1kko Does it make sense to just extract the trajectory coordinates and the structure along with some metadata (temperature, time_step), and load the SimulationData directly from there? I think that will be way more efficient (couple of MBs) than storing the entire vasprun.xml / OUTCAR (~500 MB).

This could be a small step towards a generic trajectory class, as in #67.

We could store the vasprun.xml in git lfs and have a simple script to generate the test data.

@v1kko
Copy link
Contributor Author

v1kko commented Jul 25, 2023

@stefsmeets it does make sense to eventually base the tests on a Trajectory Class.

I think that we should still have tests to convert a vasprun.xml / OUTCAR into a Trajectory class for a while, (so a bit more then a simple script) since that does influence our workflow.

Then after we merge the Trajectory class into pymatgen, we could drop those tests I guess, as the tests will be in pymatgen

Currently the vasprun.xml and OUTCAR are loaded from git lfs, this takes 0 seconds (well 6, but thats also counting the git checkout) in the Github Action, so I think we should save this for another pull request, where we can focus fully on a Trajectory class.

@v1kko
Copy link
Contributor Author

v1kko commented Jul 25, 2023

We can remove the OUTCAR file from git lfs as it is not used

@v1kko
Copy link
Contributor Author

v1kko commented Jul 26, 2023

We should move the git lfs from github to some stable provider, as we are now already over the 1GB bandwidth limit

@v1kko
Copy link
Contributor Author

v1kko commented Jul 26, 2023

I will create a separate branch with the vasprun.xml (compressed) included. Then we can use this in testing, and eventually we can throw the branch away if we do not need it anymore, in that way it will not pollute the main branch and all future pulls.

@v1kko v1kko marked this pull request as ready for review July 26, 2023 09:12
@v1kko v1kko changed the title add short run to git lfs add short run to git submodule Jul 26, 2023
@stefsmeets
Copy link
Contributor

stefsmeets commented Jul 26, 2023

I found the bug with e_act. In the code I set atom_locations to occupancy. This is not right. atom_locations in turn is used to calculate e_act.

GEMDAT/src/sites.py

Lines 111 to 112 in 58ecff7

self.atom_locations = self.sites_occupancy # Is this correct? TODO: check
self.atom_locations_parts = self.sites_occupancy_parts # Is this correct? TODO: check

@stefsmeets
Copy link
Contributor

From my point of view this PR is ready to be merged. Once pymatgen gets updated we should re-run the tests and see if they pass. We can do the refactoring in a follow-up PR.

@v1kko
Copy link
Contributor Author

v1kko commented Jul 27, 2023

We can update the dependency to https://github.com/stefsmeets/pymatgen/tree/label-update ? we can change it back later for release time

@stefsmeets
Copy link
Contributor

The downside is that this will require some compilation of pymatgen extensions on the CI.

@v1kko
Copy link
Contributor Author

v1kko commented Jul 27, 2023

I think we cache the environment with every pyproject.toml update so only one compilation needed, I will try it out

@v1kko
Copy link
Contributor Author

v1kko commented Jul 27, 2023

We have a bug in Github Action caching, lets fix that as well:

Warning: Path Validation Error: Path(s) specified in the action for caching do(es) not exist, hence no cache is being saved.

@v1kko v1kko merged commit 875be07 into main Jul 27, 2023
3 checks passed
@v1kko v1kko deleted the lfs branch July 27, 2023 07:49
@v1kko v1kko mentioned this pull request Jul 27, 2023
2 tasks
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.

enable the test cases with a short run on Github Action
2 participants