feat: add ndarray/base/diagonal#11851
feat: add ndarray/base/diagonal#11851headlessNode wants to merge 4 commits intostdlib-js:developfrom
ndarray/base/diagonal#11851Conversation
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown
status: passed
- task: lint_package_json
status: passed
- task: lint_repl_help
status: passed
- task: lint_javascript_src
status: passed
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: passed
- task: lint_javascript_tests
status: passed
- task: lint_javascript_benchmarks
status: passed
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: passed
- task: lint_typescript_tests
status: passed
- task: lint_license_headers
status: passed
---
headlessNode
added
Feature
Coverage Report
The above coverage report was generated for the changes in this PR. |
Co-authored-by: Athan <kgryte@gmail.com> Signed-off-by: Athan <kgryte@gmail.com>
Co-authored-by: Athan <kgryte@gmail.com> Signed-off-by: Athan <kgryte@gmail.com>
| sh = getShape( x ); | ||
| st = getStrides( x ); | ||
| N = sh.length; | ||
|
|
There was a problem hiding this comment.
As stated in stdlib-js/metr-issue-tracker#480 and as done in ndarray/base/rot90, it is not clear why we are not validating dims. Furthermore, it is not clear why we are not using to-unique-normalized-indices.
There was a problem hiding this comment.
Also similar to rot90, we should be validating that the input array has at least two dimensions given that dims must contain exactly two elements.
| sr = st[ d[0] ]; | ||
| sc = st[ d[1] ]; | ||
|
|
||
| // Resolve the positive and negative parts of `k`: |
There was a problem hiding this comment.
It isn't clear what is meant by "positive and negative parts". What are you meaning by this?
It is better to speak about "row" and "column" offset/index. That is, after all, what we say in the docs.
| L = sh[ d[0] ] - kn; | ||
| if ( sh[ d[1] ] - kp < L ) { | ||
| L = sh[ d[1] ] - kp; | ||
| } |
There was a problem hiding this comment.
This is a bit odd. First you set it. Then, you compute the other. And then you overwrite. I believe what you are wanting is the min. I suggest being clearer in your code.
| // Adjust the offset so that we point to the first element along the specified diagonal: | ||
| offset = getOffset( x ) + ( kp*sc ) + ( kn*sr ); | ||
|
|
||
| // Drop the specified dimensions and append the diagonal dimension: |
There was a problem hiding this comment.
This isn't the most intuitive in the sense that the dimension is appended. This matches NumPy's logic: https://github.com/numpy/numpy/blob/main/numpy/_core/src/multiarray/item_selection.c#L2319.
And I suppose it makes sense if you think about in terms of, e.g., a physical 3D cube. If you want the stack of diagonals for the first two dimensions, you take a 2D slice and then rotate it, and have it be such that the diagonal elements are always in the last dimension.
Nothing to be be done for this comment; just trying to visualize the result.
| // MODULES // | ||
|
|
||
| var toNormalizedIndices = require( '@stdlib/ndarray/base/to-normalized-indices' ); | ||
| var getDType = require( '@stdlib/ndarray/dtype' ); |
There was a problem hiding this comment.
This is a "base" package. Why are you using top-level attribute packages?
| * - The `writable` parameter **only** applies to ndarray constructors supporting **read-only** instances. | ||
| * | ||
| * @param {ndarray} x - input array | ||
| * @param {IntegerArray} dims - dimension indices defining the plane in which to extract the diagonal |
There was a problem hiding this comment.
| * @param {IntegerArray} dims - dimension indices defining the plane in which to extract the diagonal | |
| * @param {IntegerArray} dims - dimension indices defining the plane from which to extract the diagonal |
Applies here and elsewhere in this PR.
kgryte
added
Needs Changes
3 participants
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes. report:
Resolves stdlib-js/metr-issue-tracker#480.
Description
This pull request:
ndarray/base/diagonalRelated Issues
This pull request has the following related issues:
ndarray/base/diagonalmetr-issue-tracker#480Questions
No.
Other
No.
Checklist
AI Assistance
If you answered "yes" above, how did you use AI assistance?
Disclosure
Primarily written by Claude Code.
@stdlib-js/reviewers