-
Notifications
You must be signed in to change notification settings - Fork 747
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
Create an integration testing suite #1324
Conversation
Sorry for sitting on this for too long, this should be ready for a review! |
Thank you for this PR. It definitely makes important contributions to the testing. However, my concern is DeepXDE now has so many codes and functionalities, so it is really difficult to cover all the tests. It is just too much work. My current thought is that we don't need to test detailed functions such as losses, and instead, we only test those example codes. If the examples work OK, then it should be fine. Any suggestions? |
Oh yes, that sounds much better. Thanks for the review and the suggestion! I'll add tests for the notebooks and revamp this PR. |
5fdfda9
to
a873a11
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be ready for review now.
This PR now updates and fixes the already existing Makefile
in examples/
to run all the example files using make run_all_examples
. I've added the same command in the CI and have parallelized the builds on different backends to save a ton of CI time. I also had to make some minor fixes in the examples to get everything working, but I have ensured that major example changes are not present in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if I should add these datasets in or if I should simply exclude these examples from testing?
Yes, this PR is huge. Could you first make a PR that changes |
Oh yes, I can split this PR. |
a9dc69a
to
82de132
Compare
130c2a6
to
dd791d8
Compare
66b3126
to
80f1c69
Compare
400c4cb
to
7a9a893
Compare
So, I would suggest still only testing import. If we need to test the examples, we can run the Makefile manually. What do you think? |
Yes, the extra time required to run all the examples is definitely not good, and I have been worried about that since the start of my work on this (which is why I decided to run all jobs in parallel, but there are only a limited number of machines available from GitHub, making this not "completely" parallel). Testing locally sounds great. I can periodically check if the Makefile is working locally and fix it if it goes stale. I will revert the changes done to the workflow file and test import on all backends sequentially (parallelly on OS and Python versions). |
Xref: #1244
This PR adds a minimal testing suite to DeepXDE. There are a few options available for testing libraries in Python and the most widely used ispytest
.pytest
will run the files in thetests/
directory (starting withtest_
) and validate that everyassert
statement passes.The tests check that the code works as intended, including the returned values, datatypes, and if the warnings and errors are raised correctly. This way, if someone now modifies one of the loss functions and ends up messing up with them, the tests will fail indicating that something wrong has gone with the particular loss function.I have currently only implemented tests for the loss function for every backend. Some of the functions are only supported on particular backends, so they are skipped while other backends are tested. I've tried my best to include edge cases and make the tests reliable, but please let me know if I can add more tests for these functions.These tests now also execute in the CI, which means every they will be executed on every PR. DeepXDE is already imported within the test module; hence, no need to explicitly test the import in the CI.I have also added some minimal configuration forpytest
inpyproject.toml
and added it as adev
dependency, so that developers can installpytest
withdeepxde
usingpip install "deepxde[dev]"
. Moredev
dependencies will be added as we move further (includingpytest-cov
andpre-commit
).Finally, looking at #1313 (comment), all the tests of a particular backend only run with the Tensor or vector of that particular backend.Once this is merged, the tests can be populated for other DeepXDE modules. I will add support for coverage after this so that we can visualise which modules are not tested DeepXDE (all of them right now, but that will change and coverage will catch that).