Skip to content

Add new tests for existing calc modes.#71

Merged
imaliyov merged 2 commits intomainfrom
calc_tests
Dec 4, 2024
Merged

Add new tests for existing calc modes.#71
imaliyov merged 2 commits intomainfrom
calc_tests

Conversation

@ltan01
Copy link
Copy Markdown
Contributor

@ltan01 ltan01 commented Nov 28, 2024

Hello,

I added and updated tests in the testsuite for all of the calc modes except dynamics-pp and dynamics-run. I also added more tests for plot_tools.py. Some of the calc modes don't have any functions besides initializing (trans, imsigma) but I included a test script and corresponding reference for those modes.

Copy link
Copy Markdown
Member

@imaliyov imaliyov left a comment

Choose a reason for hiding this comment

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

Great work Laurie!

For plotting tests, do we actually test the PDF generated? If we do, I do not recommend this. We were trying a lot previously and it was always unstable and hardly reproducible.

I even suggest we remove any PDF testing from the current testsuite, because sooner or later they will break.

@ltan01
Copy link
Copy Markdown
Contributor Author

ltan01 commented Nov 28, 2024

Great work Laurie!

For plotting tests, do we actually test the PDF generated? If we do, I do not recommend this. We were trying a lot previously and it was always unstable and hardly reproducible.

I even suggest we remove any PDF testing from the current testsuite, because sooner or later they will break.

Thank you, Ivan!

No, I do not test the PDF generated for any of the plotting tests. I followed the format for test_plot_dispersion() in test_plot_tools.py, where the function is called but no files are generated.

@imaliyov
Copy link
Copy Markdown
Member

Great work Laurie!
For plotting tests, do we actually test the PDF generated? If we do, I do not recommend this. We were trying a lot previously and it was always unstable and hardly reproducible.
I even suggest we remove any PDF testing from the current testsuite, because sooner or later they will break.

Thank you, Ivan!

No, I do not test the PDF generated for any of the plotting tests. I followed the format for test_plot_dispersion() in test_plot_tools.py, where the function is called but no files are generated.

Sounds good!

Why do we have tests failing, for example, with errors like this one:

ERROR test_spins_calc_mode.py::test_plot_spins[1-True-False] - AttributeError: module 'perturbopy.postproc' has no attribute 'Spins'

@ltan01
Copy link
Copy Markdown
Contributor Author

ltan01 commented Dec 1, 2024

Great work Laurie!
For plotting tests, do we actually test the PDF generated? If we do, I do not recommend this. We were trying a lot previously and it was always unstable and hardly reproducible.
I even suggest we remove any PDF testing from the current testsuite, because sooner or later they will break.

Thank you, Ivan!
No, I do not test the PDF generated for any of the plotting tests. I followed the format for test_plot_dispersion() in test_plot_tools.py, where the function is called but no files are generated.

Sounds good!

Why do we have tests failing, for example, with errors like this one:

ERROR test_spins_calc_mode.py::test_plot_spins[1-True-False] - AttributeError: module 'perturbopy.postproc' has no attribute 'Spins'

I included tests for the spin calc modes! This branch is based off the main branch that doesn’t have those calc modes yet. The error message should go away once my PR for the spin calc modes is approved and the spin branch is merged.

@imaliyov
Copy link
Copy Markdown
Member

imaliyov commented Dec 2, 2024

I think the correct way would be to create another branch from the one where the spin calc mode is implemented and put tests there, then create a pull request.

Otherwise, bypassing the github tests is a bad practice. We created them exactly for this reason - to check if the new implementation works.

@hurricane642
Copy link
Copy Markdown
Collaborator

Hey @ltan01 ,
That's great, thank you a lot!
I agree with the Ivan, let's split this in 2 separate PRs!

@ltan01
Copy link
Copy Markdown
Contributor Author

ltan01 commented Dec 2, 2024

Ok, thank you all for your feedback! I'll split them into two PRs with the right branches for each one :)

@ltan01 ltan01 changed the title Add new tests for old and new calc modes. Add new tests for existing calc modes. Dec 2, 2024
Copy link
Copy Markdown
Collaborator

@hurricane642 hurricane642 left a comment

Choose a reason for hiding this comment

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

LGTM
Great work, thank you!

@imaliyov imaliyov merged commit 79a0425 into main Dec 4, 2024
@imaliyov imaliyov deleted the calc_tests branch December 4, 2024 09:48
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