Skip to content

Matrix subset according to type of input #3485

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

Open
wants to merge 10 commits into
base: v15
Choose a base branch
from

Conversation

dvd101x
Copy link
Collaborator

@dvd101x dvd101x commented May 31, 2025

Hi Jos,

This addresses #2344

It changes the behavior of index to accommodate for scalar indices and the size of the output depends on the input.

I'm still doing some tests and reviewing what documentation needs to change, but it all seems to work according to:
#2344 (comment)

A = [1,2,3; 4,5,6]
A[2,3]  --> 6
A[[1,2],[2,3]] --> [2,3; 5,6]
A[[2],[2,3]] --> [[5, 6]]
A[2,[2,3]] --> [5,6]
A[[1,2],[2]] --> [2; 5]  // which is the same as [[2], [5]]
A[[1,2],2] --> [2,5]
A[[2], [2]] --> [[5]]
A[2, [2]] --> [5]
A[[2], 2] --> [5]
A[[],[]] --> []
A[1,[]] --> []
A[[],1] --> []

@dvd101x
Copy link
Collaborator Author

dvd101x commented May 31, 2025

I think it's mostly ready for review.

Just wanted to review the following,

The elements of an index are called ranges like index(range1, range2) but the ranges are not always Range. Sometimes ranges are number, string, bigNumber, Array, Matrix, Array of Booleans, Matrix of Booleans or Range.

So I was thinking if in the documentation and maybe in the code to find a more general name for the elements of an index like index(indexElement1, indexElement2).

As a reference in numpy it's more like you slice with indices and there is no index class. So you slice A[index1, index2]

https://mathjs.org/docs/reference/functions/index.html

@josdejong josdejong changed the base branch from develop to v15 June 4, 2025 08:45
@josdejong josdejong added this to the v15 milestone Jun 4, 2025
@josdejong josdejong mentioned this pull request Jun 4, 2025
6 tasks
@josdejong
Copy link
Owner

The code looks really neat, thanks David!

Some thoughts:

  1. This is a breaking change, therefore I've changed this PR to merge into the v15 branch, and listed it at #3453.

  2. So I was thinking if in the documentation and maybe in the code to find a more general name for the elements of an index like index(indexElement1, indexElement2).

    Maybe we can call the arguments index(dim1, dim2, ...)?

  3. Does this PR have impact on the performance of subset?

  4. I'm a bit concerned about how hugely breaking this change is. Existing code will probably still work but now return differently dimensioned matrices, which can give subtle and hard to debug errors. Can we somehow help people migrate, or detect "old" usage and warn the user? Or make it backward compatible with a config flag?

@dvd101x
Copy link
Collaborator Author

dvd101x commented Jun 4, 2025

Thanks for your review Jos,

  1. Ok.
  2. Good idea! that aligns better with the rest of the code base.
  3. Makes a few of the tests slightly faster within the margin of error, but the main speed benefit might be the Matrix creation during the creation of Index, which is outside the scope of the subset benchmark. I will add some tests including the Index creation.
  4. I agree it's a big change. I'm not sure how to proceed. I'll give it some thought.

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.

2 participants