Skip to content

Bugfixes to image morph and nearest neighbors - #1595

Merged
shehzan10 merged 7 commits into
arrayfire:hotfix-3.4.1from
pavanky:minmaxfix
Sep 21, 2016
Merged

Bugfixes to image morph and nearest neighbors#1595
shehzan10 merged 7 commits into
arrayfire:hotfix-3.4.1from
pavanky:minmaxfix

Conversation

@pavanky

@pavanky pavanky commented Sep 19, 2016

Copy link
Copy Markdown
Member

@pavanky pavanky added this to the v3.4.1 milestone Sep 19, 2016
@pavanky pavanky added the bug label Sep 19, 2016
- Fixes issues with erode and dilate at corner cases
@pavanky pavanky changed the title BUGFIX: Change the initial values for min and max operations Bugfixes to image morph and nearest neighbors Sep 19, 2016
@pavanky

pavanky commented Sep 19, 2016

Copy link
Copy Markdown
Member Author

@shehzan10 I am not making my PRs run tests run automatically. Can you launch and run the tests when possible

@shehzan10 shehzan10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Adding tests 👍

@shehzan10

Copy link
Copy Markdown
Member

build arrayfire linux ci
build arrayfire osx ci
build arrayfire windows ci

Float values that are inf are outputted as inf to stringstream.
However inf isn't available in opencl, causing min and max to fail.

This alias is the easiest work around for now.
@pavanky

pavanky commented Sep 19, 2016

Copy link
Copy Markdown
Member Author

@shehzan10 you can kill old windows build. I missed something earlier by not testing OpenCL backend.

@pavanky

pavanky commented Sep 19, 2016

Copy link
Copy Markdown
Member Author

@umar456 @shehzan10
Ideally the options used in OpenCL backend should be a daughter class of stringstream with customizations. This will get rid of hacks like the last commit I have here and other work arounds like (toNum).

May be the payoff is not that great, but that is something to consider going forward.

Comment thread src/backend/cuda/math.hpp
template<> __device__ short limit_max<short>() { return 0x7fff; }
template<> __device__ short limit_min<short>() { return 0x8000; }
template<> __device__ ushort limit_max<ushort>() { return ((ushort)1) << (8 * sizeof(ushort) - 1); }
template <typename T> __device__ T maxval() { return 1u << (8 * sizeof(T) - 1); }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm. I liked the old name

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I did not want it to be confused with numeric limits. I could change it back if you feel strongly about.

@umar456 umar456 Sep 19, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't it doing the same thing as numeric limits? If anything it should have the same interface.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It has different behavior than numeric limits for floats and doubles. It had same behavior before, but this commit changed the behavior so I changed the name as well.

s_dist[sid] = dist;
s_idx[sid] = s_idx[sid + i];
}
__syncthreads();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't this still undefined?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It was already like that (look at the left). I fixed it in one place but missed this. I'll fix it.

@shehzan10

Copy link
Copy Markdown
Member

build arrayfire windows ci

@shehzan10

Copy link
Copy Markdown
Member

build arrayfire windows opencl ci

@shehzan10
shehzan10 changed the base branch from devel to hotfix-3.4.1 September 20, 2016 20:11
@shehzan10
shehzan10 merged commit ef70f35 into arrayfire:hotfix-3.4.1 Sep 21, 2016
@pavanky
pavanky deleted the minmaxfix branch September 21, 2016 01:15
@pavanky

pavanky commented Sep 21, 2016

Copy link
Copy Markdown
Member Author

@shehzan10 I missed the change in __syncthreads like @umar456 mentioned. Can you fix it in a different PR.

BTW, it would also be better if you guys start commenting this as reviews.

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.

3 participants