- Notifications
You must be signed in to change notification settings - Fork338
Fix infinite recursion and off-by-one error in triu/tril#1418
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
src/tri.rs Outdated
| res | ||
| } | ||
| if is_layout_f(&self.dim, &self.strides) && !is_layout_c(&self.dim, &self.strides) { |
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.
Thanks for the work on this.
Why is there a conditional on memory layout? This purely a performance optimization right? Great place to have a comment to say what it's doing.
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.
Yes it is - I've added a comment detailing the purpose and the point of the two other&& conditions.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Nice improvements and fixes, lgtm if you don't want to make further changes.
Would you like to squash it down into separate clean commits? Otherwise I can integrate it with a squash merge, same thing, and that also works.
Give me one minute to rebase so my commits are signed (switched computers and forgot to turn signing on) and then you can go ahead and squash merge. |
I'm not sure why it said it was out of sync with master, maybe because of ci.yaml changes in master. Rebased. |
1df6c32 intorust-ndarray:masterUh oh!
There was an error while loading.Please reload this page.
Also makes the documentation a little nicer to read, since these are such visual methods.
Closes#1415