Skip to content
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

Shadows Op #561

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Shadows Op #561

wants to merge 4 commits into from

Conversation

gselzer
Copy link
Contributor

@gselzer gselzer commented Jun 14, 2018

This Op provides an Op version of the ImageJ Core Plugin created by Barry Dezonia.

Below is a demo of the new Shadows Op (middle) run on this image (left), taken from imagej.net, alongside the output of the same image run through the Plugin (right).

shadowscomparison

Points of Discussion:

  • As of now the Op generates its Shadows kernel through the used of an angle Parameter, allowing for a single Op as opposed to the eight different Plugins that already exist. The use of an angle parameter also allows the user to generate shadows at any desired angle instead of being limited to the eight predefined angles. The only downside to this is that in order to keep the algorithm simple the Op uses sine and cosine values to generate the kernel which differs from the linear kernel of the older Plugins, resulting in differing outputs (though in most cases not visually distinguishable). Is it okay that the Op differ from the Plugin it adapts in order to increase its flexibility?

TODO:

  • Write a test for the Op. The above point should be decided on before a test is written. If we decide that the difference in output should be allowed then a simple Regression test can be created, otherwise we could test the Op based on the output of the Plugin if we deem it necessary to make the output exactly the same.

WIP: Discuss usage of unit circle for shadows kernel
These can just be generated beforehand
Removed testing printlines and imports
@ctrueden
Copy link
Member

ctrueden commented Jun 26, 2018

I am OK with the output differing from past implementations, so that we achieve a configurable angle.

Please write a test, and then we can merge it.

@gselzer
Copy link
Contributor Author

gselzer commented Jan 11, 2019

@ctrueden I found test commit that was for some reason not pushed. The test passes and is sound, however it introduces a test-scope dependency on scijava-io-http that I am not sure we want. It also forces the download of abe.tif, which seems bad to me. However it does have a method downloadOnce that seems valuable; maybe it should go in DownloadService. Let me know what you think of this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants