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

Multidimensional Resampling #33

Merged
merged 22 commits into from Aug 8, 2018
Merged

Multidimensional Resampling #33

merged 22 commits into from Aug 8, 2018

Conversation

kaifox
Copy link
Member

@kaifox kaifox commented Jul 14, 2018

This contains a first (working! ;-) version of multidimensional resampling. Currently there are two available resamplers (a resampler always treats one dimension):

  • Repeating
  • Linear interpolation (which works only for tensors of field elements)

The simplest starting point is the one in tensorics (which only supports resamplers which do not depend on a field), thus only supports repeating for the moment ;-). Assume a Tensor with two dimensions (String.class, Integer.class). Then one can do now:

Tensoric<T> resampled = Tensorics.resample(tensor).repeat(String.class).then().repeat(Integer.class).toTensoric();

If you want to use the interpolation, then one has to use the variant from TensorSupport, e.g.:

Tensoric<Double> resampled = TensoricDoubles.resample(tensor).repeat(String.class).then().linear(Integer.class, Integer::doubleValue).toTensoric();

Good starting points to see more examples and to understand the effects are the two tests:

Please also note the new interface Tensoric which you basically can see as a Tensor, without a fixed shape. Still it has a fixed set of dimensions ;-) (To be considered if more functionality, like dimensionality should go there ...)

In the review/comments, please focus (mainly) on commenting on the general concept, naming, syntax... For little things (e.g. null check here, there ... formatting ... comment)... contributions are very welcome ;-)

Additionally, please look out for loopholes... i.e. it might well be that there should be more checks introduce in the dsl (or another level), ... e.g. it makes no sense to resample a dimension if it is not contained in the tensor... to be seen on which level ...

Otherwise: Any suggestions/comments welcome

@codecov
Copy link

codecov bot commented Jul 14, 2018

Codecov Report

Merging #33 into master will increase coverage by 1.34%.
The diff coverage is 82.41%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #33      +/-   ##
============================================
+ Coverage     51.44%   52.78%   +1.34%     
- Complexity     1310     1391      +81     
============================================
  Files           343      350       +7     
  Lines          5565     5776     +211     
  Branches        770      776       +6     
============================================
+ Hits           2863     3049     +186     
- Misses         2513     2537      +24     
- Partials        189      190       +1
Impacted Files Coverage Δ Complexity Δ
...rics/core/tensor/coordinates/PositionOrdering.java 56.52% <ø> (ø) 11 <0> (ø) ⬇️
...rg/tensorics/core/resolve/resolvers/Resolvers.java 100% <ø> (ø) 7 <0> (+1) ⬆️
...a/org/tensorics/core/lang/ManipulationOptions.java 83.33% <ø> (-16.67%) 2 <0> (ø)
...va/org/tensorics/core/expressions/Placeholder.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...ore/quantity/conditions/QuantityLessPredicate.java 28% <ø> (ø) 4 <0> (ø) ⬇️
...e/functional/expressions/FunctionalExpression.java 30% <ø> (ø) 4 <0> (ø) ⬇️
...cs/core/booleans/operations/BooleanOperations.java 80% <ø> (-20%) 12 <0> (ø)
...e/expressions/AnyIterableConversionExpression.java 100% <ø> (ø) 1 <0> (ø) ⬇️
...ics/core/expressions/UnaryOperationExpression.java 0% <ø> (ø) 0 <0> (ø) ⬇️
.../quantity/conditions/QuantityGreaterPredicate.java 25% <ø> (ø) 4 <0> (ø) ⬇️
... and 98 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e359cc...591dc66. Read the comment docs.

Copy link
Member

@michi42 michi42 left a comment

Choose a reason for hiding this comment

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

Looks very nice to me! One conceptual question on the Tensoric - I don't think it's clear from the code that a Tensoric is supposed to be bound to a certain dimension set, as the dimension set is by no way obtainable from the interface ... In principle with this interface one could query arbitrary positions (with arbitrary dimensions). Should we formalize this somehow?

@@ -134,12 +145,16 @@ public Position apply(Position position) {
return Position.of(builder.build());
}

public Position from(Position position) {
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 this delegation method is just to get a nicer/more fluent syntax elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed this is the reason ... I thought it is not disturbing too much ...
you can now write
Position strippedpos = strip(ACoordinate.class).from(aPosition);


// @formatter:off
/*
* This resamples the tensor by repeating first in the Integer and then in the
Copy link
Member

Choose a reason for hiding this comment

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

This one has the comment swapped with the test above...?

Copy link
Member Author

Choose a reason for hiding this comment

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

Damn ... Never copy paste ;-) I will correct... Thanks!

@kaifox
Copy link
Member Author

kaifox commented Jul 15, 2018

I fully agree with Michis comment on the tensoric. I thought in similar ways, but left out this, as it was not immediately necessary ... and was not exactly shure how to implement it. I see the following options:

a) Extract an interface from shape, which would contain the following methods:

  • int dimensionality();
  • contains(...)
  • Set<Class<?>> dimensions(); (BTW I would like to introduce this as a replacement for dimensionSet(), as it is more conform to our other namings (and deprecate dimensionSet())
    The name of the interface could be e.g. Dimensional
    (NOTE: If the contains(..) method belongs here is not fully clear ... probably better a dedicated interface....

Shape and Tensoric would then implement this... (and thus also the tensor, which might be convenient in any case and simply delegate to shape())

a) Optionally, we could have a similar interface/object e.g. called Dimensionality which could be returned from Tensoric by a dimensionality() method....

c) We could strip down the Shape interface (which anyhow should be created) to those methods and e.g. rename the actual Shape (with the positions() method) to something like a FiniteShape....
Tensoric would then have a method:
Shape shape();
and Tensor could then override this by:
FiniteShape shape();

I think I like c) for the moment ... probably with a combination of a) so that we have the convenience methods in Tensor + Tensoric.

... still undesided ... input welcome ;-)

@kaifox kaifox merged commit 74381df into master Aug 8, 2018
@kaifox kaifox deleted the completion-by-repetition branch August 8, 2018 14:34
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.

None yet

2 participants