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

Implement long[] getDimensions()? #244

Closed
haesleinhuepf opened this issue Feb 4, 2019 · 24 comments
Closed

Implement long[] getDimensions()? #244

haesleinhuepf opened this issue Feb 4, 2019 · 24 comments

Comments

@haesleinhuepf
Copy link

Dear *,

assume, I would like to make a copy or convert some RAI, I'm very often typing this:

RandomAccessibleInterval rai = ...;
long[] dimensions = new long[rai.numDimensions()];
rai.dimensions(dimensions);
RandomAccessibleInterval<BitType> result = ArrayImgs.bits(dimensions);

Might it be possible to implement a getDimensions() method somewhere to allow writing this shorter?

RandomAccessibleInterval<BitType> result = ArrayImgs.bits(rai.getDimensions());

Or is there an alternate way of doing this?

Thanks!

Cheers,
Robert

@acardona
Copy link
Contributor

acardona commented Feb 4, 2019

There's: long[] Intervals.dimensionsAsLongArray(Dimensions) ... and note that RandomAccessibleInterval, or any Interval, extends Dimensions.

@tpietzsch
Copy link
Member

@acardona you beat me by 10 seconds...

@haesleinhuepf
Copy link
Author

Would it be possible to implement it into the Interval base class? For rookies it's impossible to know that entry point for searching for methods....

@haesleinhuepf
Copy link
Author

Oh and thanks btw! ;-)

@acardona
Copy link
Contributor

acardona commented Feb 4, 2019

To be fair, I am equally annoyed by the length of the Intervals.dimensionsAsLongArray. In my jython scripts, I sometimes import it like:

from net.imglib2.util.Intervals import dimensionsAsLongArray as getDimensions

.. or some other, shorter yet equally clear alias.

@haesleinhuepf
Copy link
Author

I use auto-completion in IntellIJ and there it doesn't help at all. :-(

image

@tpietzsch
Copy link
Member

Possibly we could put it as a default method into the Dimensions interface.
But maybe a better solution would be to make a ArrayImg.bits(Dimensions) variant

@haesleinhuepf
Copy link
Author

haesleinhuepf commented Feb 4, 2019

@tpietzsch

But maybe a better solution would be to make a ArrayImg.bits(Dimensions) variant

I also create images of non-imglib2-types and there I need that long[] array...

Thus, I would love to have the getDimensions() default method :-)

@acardona
Copy link
Contributor

acardona commented Feb 4, 2019

I second that. A long[] Dimensions.dimensions()method would be super convenient, which would be inherited by all Interval classes.

@tpietzsch
Copy link
Member

I actually agree.
For consistency, I would also add long[] max() and long[] min()default methods toInterval`.
@axtimwalde would you be okay with that?

@tpietzsch
Copy link
Member

@maarzt Do you have an opinion?

@acardona
Copy link
Contributor

acardona commented Feb 4, 2019

That'd be great. Actually, a major issue now in imglib2 is lack of discoverability. Teaching novices about net.imglib2.util.Intervals and net.imglib2.util.IntervalIndexer is a must, followed by Converters, Views and RealViews (still weird that the latter two are separate). At least the two util classes could have some of their methods implemented as default in the interfaces that they service to ameliorate the issue, as discussed here.

@axtimwalde
Copy link
Member

Perfectly fine with adding this as default implementations into the interfaces. When ImgLib2 was created, default implementations in interfaces did not exist and I wanted to limit the burden of implementing everybody's favorite helper method in a new class as much as possible. Default implementations make this consideration irrelevant.

@maarzt
Copy link
Contributor

maarzt commented Feb 7, 2019

Yes, we obviously totally add the long[] dimensions() method to the interface Dimensions. And the long[] min() and long[] max() methods in Interval. And long[] location() method in Localizable.
Buts that actually only the short answer to the question:

The Longer Answer: If you think about the naming of the methods it becomes interesting: Dimensions.dimensions() that's a stupid name. If that's not obvious to you, just think of String.string() or Iterator.iterator(). To find a good name for that method one could ask: What would this Dimensions.dimensions() method do? It would convert from Dimensions to long[]. So it would be best named Dimensions.toArray() just like 'Object.toString()' or List.toArray(). But this would be harmful in ImgLib2, because we don't want to have this method for and image: Img.toArray() should do something completely differently. Instead we might stick with Dimensions.dimensions() or the longer Dimensions.dimensionAsLongArray().

For the method min() and max() of an Interval we need to think a bit longer. Because it will be the only chance to add a method min or max with no arguments to Interval, RandomAccessibleInterval, IterableInterval, Img, ImgPlus or Dataset. So what would this min() do? It returns the minimum location of the Interval. Should the return type maybe be Localizable and not long[]. Shouldn't it be named intervalMin such that the one writes Img.intervalMin(). Or we just stick with Intervals.minAsLongArray(image).

Finally the Localizable interface could also have a methods long[] locationAsLong() and double[] locationAsDouble().

@tpietzsch
Copy link
Member

@maarzt All good points. Thanks!

It might be safe to add Localizable.toLongArray(). I cannot immediately think of a place where we would run into a situation similar to Img.toArray(). Then if Interval.min() returns Localizable, one could write img.min().toLongArray() which reads quite nice.

On the other hand, then there is a mismatch between img.dimensions(): long[] and img.min(): Localizable. So it might be best to stick to the verbose names (dimensionsAsLongArray(), minAsLongArray()).

@acardona
Copy link
Contributor

acardona commented Feb 7, 2019

I vote for the verbose names. Same as now in Intervals, and also self-documenting. They are all one control-space away anyway.

@hanslovsky
Copy link
Member

Very good points @maarzt

I vote for the verbose names. Same as now in Intervals, and also self-documenting. They are all one control-space away anyway.

Agreed.

As an addendum, what I like to do in such cases, is to introduce methods like this:

long[] min(long[] min);
int[] min(int[] min);
<P extends Positionable> P min(P min);

Unfortunately, this would change the return types of these methods. Whether or not this is a breaking change is discussed here, in particular this answer that says: Unfortunately, yes, changing a void method to return something is a breaking change.

@tpietzsch
Copy link
Member

Ok, great, let's go with the verbose method names then! Anybody wants to make a PR?

@hanslovsky I would also be happy with

long[] min(long[] min);
int[] min(int[] min);
<P extends Positionable> P min(P min);

It's definitely a breaking change. It will not only require adaption of the implementations of these methods in downstream projects. These adaptions are trivial, but still, it could be lot of places... If you ant to tackle this, you could make this a (separate) PR and we collect a few of those for next time we break API...

@hanslovsky
Copy link
Member

If you ant to tackle this, you could make this a (separate) PR and we collect a few of those for next time we break API...

Not in the near future, unfortunately, pretty busy right now. But I might get back to it sometime around fall this year.

@axtimwalde
Copy link
Member

Has this happened yet?

@imagesc-bot
Copy link

This issue has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/useful-or-interesting-functions-for-imglib2-tests-and-examples/40703/7

@bogovicj
Copy link
Contributor

bogovicj commented Jul 29, 2020

@haesleinhuepf ,

@axtimwalde and I just finished a PR that adds long[] Dimensions.dimensionsAsLongArray() as well as other, related convenience methods:

  • long[] Interval.minAsLongArray()
  • long[] Interval.maxAsLongArray()
  • double[] RealInterval.minAsDoubleArray()
  • double[] RealInterval.maxAsDoubleArray()
  • long[] Localizable.positionAsLongArray()
  • double[] RealLocalizable.positionAsDoubleArray()

@axtimwalde
Copy link
Member

@haesleinhuepf Please teach your students that using these methods frequently (e.g. per pixel) will make their code slow and sluggish because they create objects on the heap. Encourage them to use the previously existing in-place variants instead.

@haesleinhuepf
Copy link
Author

Good point, @axtimwalde. In fact I'm using this most often to create images of given size in tests and when talking to GPUs (See my initial example). Thanks anyway, that hint makes a lot of sense.

Also thanks @bogovicj and others involved for making this happen 🙂

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

8 participants