-
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
Add checks of output to tests #231
Conversation
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.
Looks good to me, except for some minor things. See my comments.
test/compare_outputs.py
Outdated
with open(file1) as f1, open(file2) as f2: | ||
diff = list(d.compare(f1.readlines(), f2.readlines())) | ||
agree = True | ||
for line in diff: |
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 would filter diff
already now, since you need it both directly below and in the log (line 77). Something like:
# Only keep lines that start with diff markers and are not comment lines
diff = [
line for line in d.compare(f1.readlines(), f2.readlines())
if line.startswith(('-','+')) and not line[2:].startswith('#')
]
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.
Good suggestion -- done.
return agree | ||
|
||
|
||
class FITSImage(object): |
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.
Please add a note that this class was copied (almost) verbatim from the Rapthor repository: rapthor/lib/fitsimage.py
. Maybe add a hyperlink to this file on GitLab as well?
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.
Done.
One more thing. I see that the loglevel is set to |
O, and when I try to run the test it fails with the following error:
Which is kind of curious, because these files seem to be present. 🤔 |
I already see what's the problem. The missing outputs are not in |
That's strange -- the files are in the right place (in |
Ah, I found the problem I think: it could write the |
I think all comments have been addressed. I also reduced the verbosity of the comparison as requested. |
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.
Looks good to me.
This PR adds checking of the outputs to the main test script (see RAP-694; it is also relevant to issue #227). The new tests compare the outputs to a reference set (stored in
tests/reference_outputs
). Currently, it is checked that:The contents of text files and catalogs are not currently checked, as the details of the fitting (e.g., Gaussian properties, number of Gaussians, their ordering, etc.) may vary due to differences in the libraries, etc. used between the test run and the run used to generate the reference outputs.
Some cleanup was also done, including the removal of old, unused test files.