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

Could regmax also work on float arrays? #47

Closed
anntzer opened this issue Aug 17, 2014 · 11 comments
Closed

Could regmax also work on float arrays? #47

anntzer opened this issue Aug 17, 2014 · 11 comments

Comments

@anntzer
Copy link

anntzer commented Aug 17, 2014

All's in the title... regmax is currently generated using SAFE_SWITCH_ON_INTEGER_TYPES_OF and checks for integer-ness of the input; is there any specific reason why it doesn't handle float types?

By the way, the docstring of regmax doesn't mention this limitation.

@luispedro
Copy link
Owner

You are right. There is no reason for this function to not work with
float arrays.

@anntzer
Copy link
Author

anntzer commented Aug 17, 2014

I think the same issue affects cwatershed (my goal right now is to simplify the "distance and watershed" example to avoid having to use stretch, which has issues when some distances are much larger than the relevant ones and thus cause the actually relevant distances to be squeezed into a single value -- yes, this can obviously be fixed by clipping the distances to a maximal relevant value, but if everything handled floats this would be a non-issue from the beginning).

@luispedro
Copy link
Owner

Yes, you are right on watershed again. The implementation currently has
integer hardcoded, but there is no reason to do it like that.

Adapting the code is actually fairly trivial. Are you available for
testing it?

@anntzer
Copy link
Author

anntzer commented Aug 18, 2014

Not until the end of the month, but after that, yes.

@anntzer
Copy link
Author

anntzer commented Sep 9, 2014

Hi, any news on that topic? I can help with testing now.

luispedro added a commit that referenced this issue Sep 10, 2014
There is no reason to restrict it to integers. The problem and code are
well-defined for any ordered type.

This was requested and discussed as issue #47 on github (not closing as
that issue also refers to watershed).
@luispedro
Copy link
Owner

Thanks for the ping. regmax/min now should work for FP images. Watershed is work-in-progress.

@luispedro
Copy link
Owner

I now added support for watershed too. I did some minimal (smoke) testing [included in unit tests], but I'd appreciate it if you gave it a thorough testing run.

@anntzer
Copy link
Author

anntzer commented Sep 14, 2014

I looked at the current tests and the best I can think of is to change test_watershed and test_mix_types to also try float entries. Anything else you would suggest?
I also tried it on one of my actual programs and it seems to work fine, but that can hardly be turned into a unit test.

@luispedro
Copy link
Owner

If it seems to be working for your data, that's OK. If you do find any issue, just open a report. Thanks.

@anntzer
Copy link
Author

anntzer commented Sep 15, 2014

Actually, the following seemingly reasonable patch to test_watershed leads to some failing tests. I haven't investigated further, though.

@@ -24,7 +24,7 @@ def test_watershed():
         [0,0,0,0],
         ])
     def cast_test(M,S,dtype):
-        M = M.astype(dtype)
+        M = M.astype(dtype if issubclass(dtype, (np.integer, int)) else int)
         S = S.astype(dtype)
         W = mahotas.cwatershed(2-S,M)
         assert sys.getrefcount(W) == 2
@@ -35,7 +35,8 @@ def test_watershed():
                [2, 2, 2, 2],
                [2, 2, 2, 2],
                [2, 2, 2, 2]]))
-    for d in [np.uint8, np.int8, np.uint16, np.int16, np.int32, np.uint32,int]:
+    for d in [np.uint8, np.int8, np.uint16, np.int16, np.int32, np.uint32, int,
+              np.float16, np.float32, np.float64, np.float128, float]:
         yield cast_test, M, S, d

@luispedro
Copy link
Owner

The markers argument should always be integer, but the other test is good and I can reproduce it.

@luispedro luispedro reopened this Sep 15, 2014
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

No branches or pull requests

2 participants