Add (P)SNR, RMSE, and MAE #536
Conversation
There was a problem hiding this comment.
I tested this (on Windows) and it works as advertised, although I currently don't have enough understanding of the underlying concept to verify the exact numbers (so I trust the unit tests).
Some minor comments (that don't have to be part of this PR):
- Would it make sense to make the
outparameter optional, or to addFunctionsignatures in addition to theseComputerones? Or is it wrong to provide a default output type like e.g.DoubleType? - From the op signature (e.g. in the Op Finder) it wasn't clear to me which of
in1andin2is the reference and which the test image. Maybe we can rename them toreferenceandtest? - Currently, these ops live in the
imagenamespace together with such diverse things asascii,distancetransform, andhistogram. I think this was discussed already, but would it make sense to create nested namespaces insideimage, or to split them into other namespaces instead?
Before merging, I suggest we squash all three commits and rebase onto current master.
|
Thanks for taking some time to look at his @imagejan!
Totally makes sense, especially since the type parameter for the output is obsolete anyway (that was a relic from when I was trying to do all computations directly on the
This would be amazing, though I fear that the information used in the Op Finder is extracted from the inherited instance variables
Good point as well, but definitely something we can discuss afterward. |
stelfrich
force-pushed
the
snr
branch
2 times, most recently
from
61119d0 to
c888b0a
Compare
I had a quick chat with @ctrueden about this and |
Adds various metrics for comparing reference images with (noisy) test images.
Maven will only execute unit tests whose class name begins or ends with "Test".
| final IterableInterval<T> reference, final IterableInterval<T> test) | ||
| { | ||
| final DoubleType result = (DoubleType) ops().run( | ||
| net.imagej.ops.image.quality.DefaultMAE.class, out, reference, test); |
There was a problem hiding this comment.
This should be ops().run(Ops.Image.MAE.class, out, reference, test);. That way if someone creates a more specific/specialized MAE op, it could be matched. Otherwise, DefaultMAE would always be used.
| final IterableInterval<T> reference, final IterableInterval<T> test) | ||
| { | ||
| final DoubleType result = (DoubleType) ops().run( | ||
| net.imagej.ops.image.quality.DefaultMAE.class, reference, test); |
| final IterableInterval<T> reference, final IterableInterval<T> test) | ||
| { | ||
| final DoubleType result = (DoubleType) ops().run( | ||
| net.imagej.ops.image.quality.DefaultPSNR.class, out, reference, test); |
There was a problem hiding this comment.
Should be ops().run(Ops.Image.PSNR.class, out, reference, test), and same idea for the remaining namespace methods you added. They should be run(...) with an interface not a concrete implementation.
| @Test | ||
| public void testPSNR() { | ||
| final DoubleType psnr = new DoubleType(); | ||
| ops.image().psnr(psnr, reference, test); |
There was a problem hiding this comment.
Rather than using the namespace methods in tests, you should use ops.run(net.imagej.ops.image.quality.DefaultPSNR.class, psnr, reference, test);. This will ensure that this unit test always tests the DefaultPSNR and not some other PSNR op.
Same idea for your other tests.
|
@stelfrich Thanks for your work on this! I modified the name of your unit tests, so maven runs them now. And have suggested a few minor edits.
I don't know if we want to wait until this is resolved before merging this, or if we're OK with merging this without? Basically it is to rename/annotate I think after the minor edits I suggested, this should be good to merge. But if you object please let me know. 😺 |
If we wait, we will be waiting for at least a few months. I'd say we should merge before that. Normally, I like the namespace method variable names to match the parameter fields, but in the case of |
|
Thanks for your comments, @awalter17! They have been addressed in two separate commits.
Feel free to merge! 👍 |
Adds implementations for (peak) signal-to-noise ratio, root mean square error, and mean absolute error of two images (a reference and a test image).
Closes #521.