Refactoring to add hyperslab selection to define an indexer

[matteodg]

Current strides are really underlying strides using just sizes to calculate them.
Adding hyperslab selection parameters: starts, strides, counts, blocks as defined in H5S_SELECT_HYPERSLAB.

[saudet]

This is looking good, thanks! Do we need to deprecate everything with sizes and strides? I think that's what 99% of users will keep using anyway. What's the downside of using the shortcuts there? Let's also merge DefaultIndex and CustomStridesIndex into say StrideIndex (no need to put an s there). BTW, it seems to me that HyperslabIndex could extend then StrideIndex. I am mistaken?

[matteodg]

I think customizing the strides is kind of advanced, that's why I thought better not to make it "easy" with a sizes and strides method.

Actually I agree on merging DefaultIndex and CustomStridesIndex into StrideIndex. And as well HyperslabIndex can extend it: I realized only lately that the strides to return when implementing Index are the underlying "physical" strides (if you have a better term for that, it's welcome!).

Moreover I'm not 100% sure we actually want to have a getter for the physical strides (it is there only for backward compatibility): what would them be needed for in case of an HyperslabIndex or any other new implementation of Index?

[saudet]

I think customizing the strides is kind of advanced, that's why I thought better not to make it "easy" with a sizes and strides method.

I see, yes, I think we can do it that way. That is, consider only sizes as something in common to all Index, and leave only the Indexer.create(long... sizes) as shortcuts taking default strides. And for anything custom, users can create an Index themselves. Sounds good.

Actually I agree on merging DefaultIndex and CustomStridesIndex into StrideIndex. And as well HyperslabIndex can extend it: I realized only lately that the strides to return when implementing Index are the underlying "physical" strides (if you have a better term for that, it's welcome!).

Moreover I'm not 100% sure we actually want to have a getter for the physical strides (it is there only for backward compatibility): what would them be needed for in case of an HyperslabIndex or any other new implementation of Index?

It's something we can add later if we need it, but anyway for sure it won't be part of the Index interface.

One more thing, let's move the factory methods to Index, perhaps something like this?

public static Index create(long... sizes) {
    return new StrideIndex(sizes);
}

public static Index create(long[] sizes, long[] strides) {
    return new StrideIndex(sizes, strides);
}

public static Index create(long[] sizes, long[] offsets, long[] hstrides, long[] counts, long[] blocks) {
    return new HyperslabIndex(sizes, offsets, hstrides, counts, blocks);
}

And maybe change it to an abstract class just to keep compatibility with Java SE 7...

[matteodg]

OK applies all suggestions, but we need Index.strides() to use it when returning the strides value in Indexer.strides(), which needs to stay to preserve backward compatibility.

[saudet]

Thanks! Yeah, I'm not too sure what I want to do with those... In any case, we don't need to create new methods that are also deprecated, that's not helpful. For example, we can remove Index.strides(), but still have a public StrideIndex.strides() that gets used in Indexer via casting.

Indexable.slice() is misleading because it doesn't take into account the original Index. Maybe we should name that Indexer.reindex()?

BTW, we can remove anything we don't need that is protected. Those are not part of the public API. For example, we should remove ONE_STRIDE and replace that with something like ONE_INDEX that gets used as a dummy index of sorts. That is useful to prevent creating useless duplicate objects.

[saudet]

We could even optimize the dummy index by creating a new OneIndex class that just returns i for everything, no strides, and use a singleton of that in all subclasses of Indexer.

[matteodg]

I'm not sure I follow you: when providing different sizes you do need to create a new instance of Index every time, no? I think I do not understand what a OneIndex would be.

[saudet]

Ah, yes, we wouldn't be able to use a singleton. In any case, the optimization would consist in having that class support only 1 size, so it doesn't need to have any strides or do any multiplications.

[saudet]

Let me know what you think of the changes I've just pushed: I made a few more things public (why not?), and "undeprecated" the factory methods in the subclasses of Indexer (because they can be considered as simple shortcuts to Index.create(), so why not leave them I guess?), and made OneIndex throw UnsupportedOperationException instead of returning unhelpful values. Thanks!

[matteodg]

Yes, I like them: I see all concepts are more consistent now and thanks for taking care of Javadocs as well!

[saudet]

Awesome! Are we good to merge this?

[matteodg]

Yup! Just wanted to add a test for HyperslabIndex, here it is.

[digiovinazzo]

@saudet I've been using the latest master version and I think we should add a method to get the actual shape of the Indexer that the Index is creating: for example the HyperslabIndex should have a shape() method to return, not the sizes used in the constructor, but the ones resulting from the hyperslab selection... And any Index really should have that. What do you think?

[digiovinazzo]

Actually the Indexer itself should have the shape() method which returns the shape() from the Index in use.

[matteodg]

Ops, it is always me: same person using personal/work account ;-)

[saudet]

If that's the maximum values of the indices, then that sounds like what "sizes" should be, and so what gets passed to the constructor is something else.

[matteodg]

You are talking about the sizes passed in the constructor of HyperslabIndex, right? If that's the case, yes, you are right those are not the maximum values of the indices, those are really the underlying sizes before the hyperslab selection.

Then it seems natural Index should have sizes() and Indexer should return that as well from its already existing sizes() method.

[saudet]

Then, yes, we'll need that.

[matteodg]

I added a few other commits here: I should have creates a new PR instead, I guess?

[saudet]

I think we can open a new PR from the same branch, but yes this one has already been merged.

[saudet]

Also please stop referencing issues in all your commits. They pollute GitHub's interface.