-
Notifications
You must be signed in to change notification settings - Fork 24
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 unit test notebooks for calwebb_detector1 #63
Conversation
A comment for myself to remember to add the following to conftest.py in the jwst/ repo:
|
@york-stsci This was my draft PR for a set of unit test notebooks that I had on hold while the other issues were sorted. I think I should just be able to add the tmpdir cell to the notebooks and then we can merge these as well, right? |
I think that they've already been added, so it might actually just make sense to close this PR. Unless there are test notebooks that are in this PR but haven't yet been added? |
Yeah, these notebooks haven't been added yet, so I left the PR as a draft. But I need to add the tmpdir cells now that we have that in place. |
So, there are a couple of other things that look as if they should be changed. If you look at the existing regression test notebooks, you'll see how I wrote the part of the script that runs the regression tests in those. As is, these notebooks will not run in the Jenkins environment, because the jwst module directory is not where they expect to find it, and also while the script creates a temporary directory, it then saves its output into a directory in the same directory as the notebook with the name "tmpdir", rather than using the temporary directory it defined. The already added regression tests also show how that works. |
K sounds good, I'll work on them. Thank you! |
Since the unit tests were accidentally merged in a different PR, I'm closing this PR. Any updates to the unit tests will be opened as new PRs. |
Before merging this PR, there are a couple issues that we have to discuss/resolve:
So, let's hold off on trying to merge this PR for now.