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

Adding 2 example tests for the pytest lesson. #4

Merged
merged 1 commit into from
Jun 19, 2024
Merged

Adding 2 example tests for the pytest lesson. #4

merged 1 commit into from
Jun 19, 2024

Conversation

sjayellis
Copy link

@orbeckst Here are the additional tests for the pytest lesson.

Changes made in this Pull Request:

  • Added a non-fixture test for charmm data intended to be referenced for an exercise.
  • Added a test for an expected failure in the extract function.

@pep8speaks
Copy link

Hello @sjayellis! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 58:1: W293 blank line contains whitespace
Line 60:1: W293 blank line contains whitespace
Line 65:1: E266 too many leading '#' for block comment

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Lgtm — optional comments on precision of fixtures (and these comments may well equally apply to anything I originally added — just occurred to me while looking at yours that it may look nicer to choose ground truth number, although just choosing the numbers that you get when you run the code is more straightforward).

I’m going to merge and if you feel like changing anything we can do this in another PR.

"gamma": 0,
"n_frames": 98,
"totaltime": 96.9999914562418,
"dt": 0.9999999119200186,
Copy link
Member

Choose a reason for hiding this comment

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

You could probably set to 1.0 as this is the “true” Timestep. I think this is only due to float32 precision looking a bit funny.

If the tests don’t pass with 1.0 then I wouldn’t change because then you have to start a discussion about the importance of doing floating point comparisons correctly—-which is an important topic but perhaps not something you want to discuss at this moment.

"beta": 0,
"gamma": 0,
"n_frames": 98,
"totaltime": 96.9999914562418,
Copy link
Member

Choose a reason for hiding this comment

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

Similar to other comment: could perhaps be set to 97.0

@sjayellis
Copy link
Author

I did a quick test, setting dt to 1 and totaltime to 97 for charmm passes tests.
Similarly, setting the dt to 100 and totaltime to 900 for gmx passes.

I do think the cleaner numbers look better, but don't have a particular preference on which we use. I intended to have the everyone copy those text blocks instead of writting them, so it should be straightforward either way.

@orbeckst
Copy link
Member

Thanks for checking!

Let's leave it with rough numbers for right now. If we want to prettify all reference numbers we could do it in a separate PR. I will go with your decision.

@orbeckst orbeckst merged commit fee5413 into MDAnalysis:main Jun 19, 2024
17 checks passed
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.

3 participants