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

added tests for MD method ENUMS in Gromacs #80

Merged
merged 3 commits into from
Jan 19, 2024

Conversation

JFRudzinski
Copy link
Collaborator

@ladinesa I added the requested tests. These each use only a single configuration of a relatively small system, so they should be efficient. In principle I can make these even smaller, but I will try to address that when I get to #30

@JFRudzinski JFRudzinski requested a review from ladinesa January 4, 2024 09:03
@JFRudzinski JFRudzinski self-assigned this Jan 4, 2024
@JFRudzinski JFRudzinski linked an issue Jan 4, 2024 that may be closed by this pull request
@JFRudzinski
Copy link
Collaborator Author

@ladinesa So, the new tests are failing in the pipeline even though they all pass for me locally. I did some debugging, and it appears to me that the new tests do not have the correct archive, somehow the data is coming from some other file/test? Very confused, I don't know how this is possible. Any ideas?

@JFRudzinski JFRudzinski force-pushed the 61-add-test-for-sd-integrator-gromacs branch from 18e9b08 to 94fd974 Compare January 19, 2024 08:22
@JFRudzinski
Copy link
Collaborator Author

@ladinesa not sure if you saw this. I just rebased the branch to see if that would help, but I am still having the same issue: pytests pass locally but the pipeline fails. I am tempted to create a new branch from develop and implement the changes again from scratch since it seems something really strange is going on, but maybe you could take a quick look first and let me know if the tests work locally for you as well?

@ladinesa
Copy link
Collaborator

@ladinesa not sure if you saw this. I just rebased the branch to see if that would help, but I am still having the same issue: pytests pass locally but the pipeline fails. I am tempted to create a new branch from develop and implement the changes again from scratch since it seems something really strange is going on, but maybe you could take a quick look first and let me know if the tests work locally for you as well?

@JFRudzinski sorry for the late reply, the problem seems to be that md.log is missing from your commit. This is because *.log is included in .gitignore. Please remove this line and add the md.log files again.

@ladinesa
Copy link
Collaborator

ladinesa commented Jan 19, 2024

Are the test files of reasonable size? If indeed I am fine with the tests and you can merge it.

@JFRudzinski
Copy link
Collaborator Author

Are the test files of reasonable size? If indeed I am fine with the tests and you can merge it.

🤦 🤦 yes, thank you. I remember now I had to do this once before. Can I leave the *.log out of the .gitignore? It was under the "Django" section.

@ladinesa
Copy link
Collaborator

Are the test files of reasonable size? If indeed I am fine with the tests and you can merge it.

🤦 🤦 yes, thank you. I remember now I had to do this once before. Can I leave the *.log out of the .gitignore? It was under the "Django" section.

I think it would not do any harm. My bad I simply copied this ignore file from nomad I guess because I am lazy hehe. We can also create an ignore file later

@JFRudzinski
Copy link
Collaborator Author

Are the test files of reasonable size? If indeed I am fine with the tests and you can merge it.

🤦 🤦 yes, thank you. I remember now I had to do this once before. Can I leave the *.log out of the .gitignore? It was under the "Django" section.

I think it would not do any harm. My bad I simply copied this ignore file from nomad I guess because I am lazy hehe. We can also create an ignore file later

That's fine. Thanks for the help. Did you want to check this or shall I merge it?

@ladinesa
Copy link
Collaborator

Are the test files of reasonable size? If indeed I am fine with the tests and you can merge it.

🤦 🤦 yes, thank you. I remember now I had to do this once before. Can I leave the *.log out of the .gitignore? It was under the "Django" section.

I think it would not do any harm. My bad I simply copied this ignore file from nomad I guess because I am lazy hehe. We can also create an ignore file later

That's fine. Thanks for the help. Did you want to check this or shall I merge it?

I am fine with it. my only concern as usual is the file size. if they are manageable i.e. < 1mb then i think everything is fine. Also if the assertions are again wrt to the ones in the output file that would also be great. But these details we can also redo again when we migrate the parsers.

@JFRudzinski
Copy link
Collaborator Author

Are the test files of reasonable size? If indeed I am fine with the tests and you can merge it.

🤦 🤦 yes, thank you. I remember now I had to do this once before. Can I leave the *.log out of the .gitignore? It was under the "Django" section.

I think it would not do any harm. My bad I simply copied this ignore file from nomad I guess because I am lazy hehe. We can also create an ignore file later

That's fine. Thanks for the help. Did you want to check this or shall I merge it?

I am fine with it. my only concern as usual is the file size. if they are manageable i.e. < 1mb then i think everything is fine. Also if the assertions are again wrt to the ones in the output file that would also be great. But these details we can also redo again when we migrate the parsers.

Yes, each test file is ~244k. In principle this could be reduced further, I have opened an issue to reduce the test file sizes in general, but I would like to try to do that together in one pass.

I did think about that, but again I would rather do this all together per parser. I will open issues about this so we don't forget.

@JFRudzinski JFRudzinski merged commit 1a62fc1 into develop Jan 19, 2024
2 checks passed
@JFRudzinski JFRudzinski deleted the 61-add-test-for-sd-integrator-gromacs branch January 19, 2024 10:37
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.

Add test for sd integrator gromacs
2 participants