Skip to content

add size(A,d) methods#3

Open
bjarthur wants to merge 10 commits intoJuliaIO:mainfrom
bjarthur:bja/size
Open

add size(A,d) methods#3
bjarthur wants to merge 10 commits intoJuliaIO:mainfrom
bjarthur:bja/size

Conversation

@bjarthur
Copy link
Copy Markdown
Member

with help from claude.

@mkitti

@mkitti
Copy link
Copy Markdown
Member

mkitti commented Apr 16, 2026

How about d == 0 ?

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds size(A, d) support for TensorStoreWrapper and IndexDomainWrapper to better match Julia’s array interface.

Changes:

  • Implement Base.size(::TensorStoreWrapper, ::Integer) with out-of-range dimensions returning 1.
  • Implement Base.size(::IndexDomainWrapper, ::Integer) similarly.
  • Extend tests to cover size(x, d) for both wrappers.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/TensorStoreWrapper.jl Adds Base.size(w, d) overloads for the wrapper types.
test/runtests.jl Adds assertions validating size(x, d) behavior (including d > ndims).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/TensorStoreWrapper.jl Outdated
Comment thread src/TensorStoreWrapper.jl Outdated
bjarthur and others added 3 commits April 16, 2026 12:34
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/TensorStoreWrapper.jl Outdated
Comment thread src/TensorStoreWrapper.jl Outdated
bjarthur and others added 3 commits April 21, 2026 17:24
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@bjarthur
Copy link
Copy Markdown
Member Author

bjarthur commented Apr 22, 2026

the tests pass for me on MacOS. i can reproduce though on ubuntu. all that is required is to using PythonCall. so i don't think the problem is with PyTensorStore.

what i don't understand though, is that in a @temp environment, if i using PythonCall, i don't see the error. it loads just fine. version is the same (v0.9.31)b as when the error is produced when using the PyTensorStore environment.

@mkitti
Copy link
Copy Markdown
Member

mkitti commented Apr 22, 2026

I merged #5 into this branch and main, which should resolve the issue.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/TensorStoreWrapper.jl Outdated
Comment thread src/TensorStoreWrapper.jl Outdated
mkitti and others added 2 commits April 23, 2026 02:10
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@mkitti
Copy link
Copy Markdown
Member

mkitti commented Apr 23, 2026

@copilot any further comments on this pull request?

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.

3 participants