-
Notifications
You must be signed in to change notification settings - Fork 2
Tensors of typed dim first iteration #37
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #37 +/- ##
===========================================
+ Coverage 52.65% 52.96% +0.3%
- Complexity 1395 1430 +35
===========================================
Files 351 360 +9
Lines 5773 5915 +142
Branches 790 803 +13
===========================================
+ Hits 3040 3133 +93
- Misses 2536 2581 +45
- Partials 197 201 +4
Continue to review full report at Codecov.
|
…t fully satisfactory.
…ttps://github.com/tensorics/tensorics-core into tensors-of-typed-dim-first-iteration # Conflicts: # build.gradle
Finally, I managed to do another attempt ... This time based on tensorbackeds ... Unfortunately, there are generic monsters appearing... Some more tests and examples to come ... still, already now any comments welcome ... would be good to merge soon, to not forget what was done ;-) ... In the end, nothing should be braking as all of the things were extensions. It seems to be clear now, that some solution for PreBuilders would be now in order ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi guys, would you be ok if I merge this one? Even if there is some mor stuff to be done, I would definitely like to avoid putting more into this PR....
* * For the moment the same calls are kept in the abstract tensorbacked... However, they could potentially be removed, | ||
* except we assume that somebody calls this constructor directly...? | ||
*/ | ||
public static <TB extends Tensorbacked<E>, E> Tensor<E> ensureSameDimensions(Class<TB> tensorbackedClass, Tensor<E> tensor) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michi42 , do you remember the reasons for the potential dimension mutation? I saw that it were your commits which introduced this (I am sure for a good reason)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remember correctly this is to properly treat inheritance in dimensions, since we allow interfaces there.
Consider interface I and class A implementing it. Then you have:
Ti = Tensorbacked(Double) with dimension I
Ta = Tensorbacked(Double) with dimension A
in this case you should be able to do DoubleTensorics.calculate(Ti).plus(Ta) which should return a Tensorbacked in dimension I.
Likewise, you should be able to create Ti as a copy of Ta.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think this makes sense. I will put this into the javadoc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the comment a bit, although I am not sure at the moment if this would not work otherwise ... but indeed it seems to be a good approach to align the dimensions.
This is a first attempt for an implementation of #36 .
there is a bit of hack stuff with proxies to avoid too many classes ... have a look ... it is not straightforward ... a simpler solution welcome ;-)
Greetings
Kajetan