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

adds integral arrays. Closes JuliaImages/ImagesFiltering.jl#4 #1

Merged
merged 5 commits into from
Feb 24, 2021

Conversation

mronian
Copy link
Contributor

@mronian mronian commented Aug 26, 2016

cc @timholy 😄

getindex(A::IntegralArray, i::Int...) = A.data[i...]

function getindex(A::IntegralArray, i::ClosedInterval...)
_boxdiff(A, left(i[1]), left(i[2]), right(i[1]), right(i[2]))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can extend this to nD later on 😄

Copy link
Member

Choose a reason for hiding this comment

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

Sure 😄

@@ -1,5 +1,78 @@
module IntegralArrays

# package code goes here
using Images, ClosedIntervals
Copy link
Member

Choose a reason for hiding this comment

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

We'll likely need to persuade him to register this package.

Copy link
Member

@timholy timholy Aug 26, 2016

Choose a reason for hiding this comment

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

Just looked over the source code. Doesn't look that friendly to writing tight loops:

  • the order-switching means there's a branch on construction
  • the testing for empty intervals is a lot less clean than the way UnitRanges work

I wonder if we should just start a tiny package of our own.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm...for Integral Arrays, I dont think anyone would index with a reverse ordered interval. Maybe we can just use Tuples instead?

Copy link
Member

Choose a reason for hiding this comment

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

That's a possibility, but I think it's a little dangerous because I suspect Jeff is mulling over whether we should ditch CartesianIndex and just use tuples for multidimensional indexing. (Certainly, I've given that some serious consideration.) If that happened, then indexing an IntegralImage with a 2-tuple would suddenly become very confusing.

So I think we need a type. Given the paralysis surrounding intervals, let's try making the nth package, I bet it will suddenly become the standard.

The hardest part will be the package name. Some thoughts:

  • SimpleIntervals
  • LRIntervals
  • OrderedIntervals
  • Intervulz 😄
  • IntervalSets
  • EnclosedIntervals

Of all these I think I like IntervalSets the best. It suggests that no arithmetic is supported. We might have the types Closed, Open, LOpen, and ROpen eventually (but perhaps start with Closed).

(Interestingly, there's a scala package with that name: https://github.com/rklaehn/intervalset)

Copy link
Member

Choose a reason for hiding this comment

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

I can do this if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll take a shot at it 😄. I assume this shouldn't be in JuliaImages...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considering that order switching and emptiness checking is the issue, what would be the way to go? Should IntervalSets be derived from UnitRanges?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure where to put it. Maybe https://github.com/JuliaMath.

And yes, I'd use the code for UnitRanges as the main inspiration: I'm thinking of these as ranges without any step. Obviously, don't mimic this:

julia> 0:-5
0:-1

just keep whatever the user specified.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling a2aa5bb on int-arr into * on master*.

@mronian
Copy link
Contributor Author

mronian commented Sep 1, 2016

Seems good to merge?


linearindexing(A::IntegralArray) = Base.LinearFast()
size(A::IntegralArray) = size(A.data)
getindex(A::IntegralArray, i::Int...) = A.data[i...]
Copy link
Member

Choose a reason for hiding this comment

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

Should we support indexing with integers at all? What use is there for the integral itself?

If we don't have to support Int, it might be good to have it throw a helpful error message (rather than just throw MethodError).

Copy link
Member

Choose a reason for hiding this comment

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

I guess my point is that folks might unthinkingly index with integers and assume it covers the range from the beginning to the indicated position.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completely agree on this point. Will change it 😄

@timholy timholy mentioned this pull request Aug 26, 2020
jakewilliami referenced this pull request in jakewilliami/IntegralArrays.jl Aug 31, 2020
This is my best attempt at adapting #1.  What do you think?
Co-authored-by: Jake Ireland <[email protected]>
@timholy timholy merged commit 1d6d148 into master Feb 24, 2021
@timholy timholy deleted the int-arr branch February 24, 2021 14:27
@timholy
Copy link
Member

timholy commented Feb 24, 2021

Another commit for @mronian! Hope you're well 😃 .

@mronian
Copy link
Contributor Author

mronian commented Jun 17, 2021

Wow I just went through all my notifications 🤣

I've been on a wild ride - founded a medical devices company (www.morphlelabs.com - we make high speed scanners for microscopy slides at a very affordable price) and served as its CTO up until last month.

Now, back to exploring the awesome things people are building on Github and figuring out what to do next!

Hope you are doing great @timholy 😄

I was lucky to have you as my mentor during GSOC 2016 🙏. Forever grateful for that experience!

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.

3 participants