Skip to content
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

Add matlab-like tril method for two-dimensional arrays #1331

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fedemagnani
Copy link

Adding trill method for two-dimensional arrays in order to create a lower triangular matrix from a given matrix. This function reproduces the behavior of the Matlab function with same name: it returns the elements on and below the kth diagonal of A

@nilgoyette
Copy link
Collaborator

I'm not against adding this feature to ndarray and I think the other maintainers would agree. This being said

  • I'm not a fan of the name. tril seems to be the standard (Matlab and Numpy), but it means nothing (maybe it's just me?)
  • I didn't take the time to think about it, but the implementation looks like it could be "better". For example, numpy does it in 3 lines (I know we can't and shouldn't do it like Numpy)

@adamreichold
Copy link
Collaborator

I'm not a fan of the name. tril seems to be the standard (Matlab and Numpy), but it means nothing (maybe it's just me?)

I guess we could just spell it out, e.g. triangular_lower?

@fedemagnani
Copy link
Author

fedemagnani commented Oct 5, 2023

I wrote this method trying to reproduce the behaviour of a matlab script which was invoking this function.

  • I'm not a fan of the name. tril seems to be the standard (Matlab and Numpy), but it means nothing (maybe it's just me?)

Like you I found the name a bit confusing at first glance, but since it generates a lower triangular from a given matrix I thought that "tri" was standing for "triangular" and "l" as "lower". I decided to keep the same name for consistency with the matlab function thinking that it could have been useful for people with my similiar use case.

  • I didn't take the time to think about it, but the implementation looks like it could be "better". For example, numpy does it in 3 lines (I know we can't and shouldn't do it like Numpy)

Regarding the length of the code, this is the way I wrote it and I think that adding a method which expands the functionalities of the package is better than not doing it :)

I don't think that this method will be used that much but if it will surge the need of optimizing it I bet that it is going to be optimized in the future

@nilgoyette
Copy link
Collaborator

I don't think that this method will be used that much but if it will surge the need of optimizing it I bet that it is going to be optimized in the future

There's no need to procrastinate, we can do it properly right now. If you're still interested, of course.

  • Direct indexing is "slow" in ndarray and that's what you use
  • You could test using a Zip::indexed(m.rows_mut()), then something like row.slice(s![?..]).fill(0.0). If I'm right, this could be done in 4-5 lines.
  • Once this is done, you might as well code triu. It doesn't make much sense to have one without the other!

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.

None yet

3 participants