Sptensor tutorial indexing #174
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Our subscripts approach was still broken * Handling indexing for a single dim tensor required some nuance
* Add verbosity to error instead of DEBUG_test flag * We don't really use pytest markers so prune them from tests I am touching
* Group indexing tests into test calss for logical separation * Make tests smaller/more focussed in scope * Caught a bug in subtensor assignement sizes * Small fix around int vs np int check (broader typing issue outside scope here)
* Group indexing tests into test class for logical separation * Make tests smaller/more focussed in scope * Small fix around int vs np int check (broader typing issue outside scope here)
* Fix some type confusion in ktensor * Move some special casing to unified helper method
This was referenced Jul 3, 2023
|
Your updates work nicely with the sptensor tutorial's TODOS! |
DeepBlockDeepak
pushed a commit
to DeepBlockDeepak/pyttb
that referenced
this pull request
Jul 3, 2023
…till needs to be cleaned in separate feature branch for the associated separate PR.
|
I'll submit an issue to discuss the example in |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is all scope creep. I've been battling indexing for a while and had been meaning to do a larger scale refactor to make things more obvious. I finally had some bandwidth for this but I think there is more to do related to simplifying and unifying code in setitem for sptensor and tensor (getitem for both tensors now shares more common logic). That will be another day.
As far as I am aware this should resolve:
sptensor()to take a shape argument to simplify generating empty sptensors with starting shape__getitem__()determine use ofextractparameter #160 pruned usage ofextractfrom tensor and sptensor since it aided in the test cleanupend()is necessary in pyttb classes #161 keptend()but updated sptensor and tensor indexing to accept negative values for non-subscript options (subscripts are most explicit so need to be correct).I broke out the different pieces by commit so it might be easier to review that way for motivation of individual changes then look at the resulting product as a whole to decide if there is anything else we'd like to change. In my opinion grouping the tests into test classes with smaller simpler test methods makes it clearer WHY we have certain indexing tests and where updated tests should go if we find more edge cases.
Some of the work here overlaps pretty strongly with the effort to add typing over all of pyttb. There are quite a few tests in our unit tests to exercise a bunch of different type combinations that I believe mypy should be able to statically validate/point out issues with as well continue our coverage.
Also for
test_subtensor_assign_oddityI believe this is a bug in upstream MATLAB tensortoolbox since it doesn't really make sense. I'd prefer to land this PR then think about it some more/open a PR to MATLAB for clarification.