DM-39622: Add plotting for images #61
Conversation
| savePlotAs=outputFilename) | ||
| self.assertTrue(os.path.isfile(outputFilename)) | ||
| self.assertTrue(os.path.getsize(outputFilename) > 10000) | ||
|
|
There was a problem hiding this comment.
I think it might be worth adding a loop to check that the various stretching options work. You don't need to save and check every plot, I don't think, but something that tests all the stretch options at least would be a good addition I think.
There was a problem hiding this comment.
Maybe manipulate one of the pixels in the image to set it to nan, too, just to check that everything is nan-safe too.
There was a problem hiding this comment.
I set a small area of the image to nans and added code to test stretches.
There was a problem hiding this comment.
That's nearly it, but None is not the same as NaN. Please set them to np.nan so that the nan-safety can be properly checked.
There was a problem hiding this comment.
Also, actually, this doesn't check that it works with all-nan images, and I think that is something that needs to at least not crash, and at present that is indeed a crash because the colorbar can't handle the limits. Once that's fixed I think this is basically good to merge (after running Jenkins and rebasing and fixing these final comments, that is)
There was a problem hiding this comment.
How do you want to proceed here? Exit with an error that nans are in the image or give a warning that nans are in the image and they will be replaced with something that the code can handle?
There was a problem hiding this comment.
If the image is all Nan there is no point in plotting. The code will check if image is pure nan and will raise ValueError.
There was a problem hiding this comment.
Some nans in the image is perfectly acceptable, and the code should (and I think already does?) handle it fine, I'm just saying that there should be a test for it.
For all-nan images there is no point in plotting, but it happens, and this code should give an all-white image rather than raising. You could do a quick check for if the entire array is nan if you like and then just return a blank image, perhaps. At this point I'm happy for you to just file another ticket for that, assign it to me, and add a TODO in the code though. Do please add the test for the some-nans case though.
There was a problem hiding this comment.
Part of this is already in the code (check if image is all nan). There is also a test for cases with few nans in the image. It is included in test for plotting.py. I will change the ValueError part to blank image.
There was a problem hiding this comment.
Yup, I just checked and that all seems to be working perfectly now, thanks.
|
Final comment is that you need to remove the merge commits on here before merging. DM uses a |
|
Rebase workflow is here: |
madamow
force-pushed
the
tickets/DM-39622
branch
from
40cd89b
to
d24137d
Compare
madamow
force-pushed
the
tickets/DM-39622
branch
from
d24137d
to
9792ba3
Compare
madamow
force-pushed
the
tickets/DM-39622
branch
from
9792ba3
to
f8941f2
Compare
|
I made all the changes; it is ready for merge. |
|
Sorry, but it is still full or merge commits. These must be removed before it can merge. |
|
Apologies - I am wrong, I'd missed that because you squashed everything into a single commit. That is certainly one approach, so yeah, I think once Jenkins passes it's good to merge. Build is running here: Once that's green you can hit merge. |
No description provided.