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

Border fix #1952

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Border fix #1952

wants to merge 5 commits into from

Conversation

akuchibotla
Copy link

Addressing issue #1669 and looking for feedback

In particular, I made sure there were no NaNs in the swirl mapping as well as no numbers less than 0. The arithmetic seemed to work incorrectly when radius was 0 (DivideByZero) but I feel as though my patch for that is still a little hacky. Is there a Numpy number abstraction that works as a "very small number"?

@stefanv
Copy link
Member

stefanv commented Feb 20, 2016

How about np.isclose?

@@ -332,6 +332,8 @@ def _swirl_mapping(xy, center, rotation, strength, radius):
# Ensure that the transformation decays to approximately 1/1000-th
# within the specified radius.
radius = radius / 5 * np.log(2)
if radius == 0:
radius = np.power(10.0, -500)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

np.finfo(float).eps

@stefanv
Copy link
Member

stefanv commented Feb 20, 2016

Let's also add a test to ensure that this remains fixed in the future.

@akuchibotla
Copy link
Author

@stefanv I'm confused about your comment to use np.isclose rather than the equality check I'm using. This error seems to occur only when the radius is set to exactly 0 so I'm not sure why checking if the radius is close to 0 would be helpful here.

@stefanv
Copy link
Member

stefanv commented Feb 20, 2016

Ah, I thought some other small numbers would also trigger the error. Have
you checked even for numbers as small as 1e-15? If this are fine then the
exactly zero check is fine.
On Feb 19, 2016 10:54 PM, "Anand Kuchibotla" notifications@github.com
wrote:

@stefanv https://github.com/stefanv I'm confused about your comment to
use np.isclose rather than the equality check I'm using. This error seems
to occur only when the radius is set to exactly 0 so I'm not sure why
checking if the radius is close to 0 would be helpful here.


Reply to this email directly or view it on GitHub
#1952 (comment)
.

@emmanuelle
Copy link
Member

@akuchibotla can you also add a test to check that the issue of #1669 is solved? Thanks!

@akuchibotla
Copy link
Author

Not 100% sure if this is the kind of test you're looking for

swirled = tf.swirl(image, strength=10, **swirl_params)
unswirled = tf.swirl(swirled, strength=-10, **swirl_params)

assert np.mean(np.abs(image - unswirled)) < 0.01
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

np.allclose?

@soupault
Copy link
Member

soupault commented Apr 9, 2016

@akuchibotla @emmanuelle What do you think about this? :

  1. Use checkerboard as in the original issue;
  2. Swirl the image;
  3. Calcualte histogram / sum of pixels over ~2-pix wide frame;
  4. Compare with reference value.

Base automatically changed from master to main February 18, 2021 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants