-
Notifications
You must be signed in to change notification settings - Fork 35
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
Mat index bug #249
Open
dingraha
wants to merge
18
commits into
jonniedie:main
Choose a base branch
from
dingraha:mat_index_bug
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Mat index bug #249
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
90226ad
WIP: debugging matrix index bug
dingraha 808177f
WIP: debugging matrix index
dingraha 7b3767c
WIP: still debugging matrix index bug
dingraha 571d349
WIP: I think this might be working...
dingraha def0aca
Cleaning up
dingraha 0db92ae
More cleanup
dingraha f6e4620
Small tweak to `reindex` for `Shaped1DAxis`
dingraha 01d7c6e
Fix up indexing with multiple symbols and `ShapedAxis`/`Shaped1DAxis`
dingraha 0d9d5d2
Remove leftover debug comment in `show.jl`
dingraha b43853b
Fix construction with empty `NamedTuples`
dingraha bec718c
WIP: trying to `stack` ComponentVectors
dingraha ab68e9a
Fix https://github.com/jonniedie/ComponentArrays.jl/issues/254
dingraha f60730a
Add more tests for `stack`
dingraha 0c99a38
Add tests for stacking issue #254
dingraha f2ec1b7
Merge remote-tracking branch 'upstream/main' into mat_index_bug
dingraha 55165ed
Remove `stack_ca`
dingraha 6d20899
Arg, remove random `t` copy-paste error
dingraha 0ba4bb1
Restrict `stack` tests to Julia >=1.9
dingraha File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
Oops, something went wrong.
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.
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.
Hm. I guess I don't understand why we'd need to wrap this in a new type. In the example you gave in the issue,
b
is a vector element, not annx1
matrix. I don't think we should introduce a newShapedAxis1d
type if all vector elements are behaving incorrectly. We should just fix the issue of adjoint/transposition that's broken directly. So I guess specifically, theShapedAxis
needs to be created from theFlatAxis
during the adjoint operation, not beforehand. And for that, we can use the normalShapedAxis
without having to introduce a new type.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.
@jonniedie My bad, just saw this comment.
Hmm... not sure I follow, but fwiw I think everything is fine with the transpose—the issue comes up later on.
The problem with the test case in issue #249, aka
appears to be due to the fact that the shape of a unit range (which is used for vector components like
X[:b]
in the example) isn't understood byComponentArrays.maybe_reshape
. The error I get:In
maybe_reshape
:during
J[:d, :b]
,axs
isi.e. the unit range is converted to a
FlatAxis
, which is then ignored when definingshapes
inmaybe_reshape
.The axes of the transpose
X'
look fine to me:The point of the
Shaped1DAxis
is to have a type that can be a part of theNotShapedOrPartitionedAxis
Union
defined inaxis.jl
.This allows it to skip the problematic and unnecessary
maybe_reshape
for vector components.If there was a way to dispatch on
ShapedAxis{Shape}
whenShape
was a length-1Tuple
then I don't think we'd needShaped1DAxis
, but I don't know of a way to do that.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.
@jonniedie Another polite ping re: this PR. :-)
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.
@jonniedie Another polite ping re: this PR.