feat: add C implementation for lapack/base/dlacpy
#5210
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
---
type: pre_push_report
description: Results of running various checks prior to pushing changes.
report:
- task: run_javascript_examples
status: na
- task: run_c_examples
status: na
- task: run_cpp_examples
status: na
- task: run_javascript_readme_examples
status: na
- task: run_c_benchmarks
status: na
- task: run_cpp_benchmarks
status: na
- task: run_fortran_benchmarks
status: na
- task: run_javascript_benchmarks
status: na
- task: run_julia_benchmarks
status: na
- task: run_python_benchmarks
status: na
- task: run_r_benchmarks
status: na
- task: run_javascript_tests
status: na
---
|
/stdlib update-copyright-years |
Coverage Report
The above coverage report was generated for the changes in this PR. |
---
type: pre_push_report
description: Results of running various checks prior to pushing changes.
report:
- task: run_javascript_examples
status: na
- task: run_c_examples
status: na
- task: run_cpp_examples
status: na
- task: run_javascript_readme_examples
status: na
- task: run_c_benchmarks
status: na
- task: run_cpp_benchmarks
status: na
- task: run_fortran_benchmarks
status: na
- task: run_javascript_benchmarks
status: na
- task: run_julia_benchmarks
status: na
- task: run_python_benchmarks
status: na
- task: run_r_benchmarks
status: na
- task: run_javascript_tests
status: na
---
…ectories
---
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: na
- task: lint_package_json
status: passed
- task: lint_repl_help
status: na
- task: lint_javascript_src
status: na
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: na
- task: lint_javascript_tests
status: na
- task: lint_javascript_benchmarks
status: na
- 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: na
- task: lint_typescript_tests
status: na
- task: lint_license_headers
status: passed
---
---
type: pre_push_report
description: Results of running various checks prior to pushing changes.
report:
- task: run_javascript_examples
status: na
- task: run_c_examples
status: na
- task: run_cpp_examples
status: na
- task: run_javascript_readme_examples
status: na
- task: run_c_benchmarks
status: na
- task: run_cpp_benchmarks
status: na
- task: run_fortran_benchmarks
status: na
- task: run_javascript_benchmarks
status: na
- task: run_julia_benchmarks
status: na
- task: run_python_benchmarks
status: na
- task: run_r_benchmarks
status: na
- task: run_javascript_tests
status: na
---
…unctions
---
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: na
- task: lint_repl_help
status: na
- task: lint_javascript_src
status: passed
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: na
- task: lint_javascript_tests
status: na
- task: lint_javascript_benchmarks
status: na
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: passed
- task: lint_c_examples
status: passed
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: na
- task: lint_typescript_tests
status: na
- task: lint_license_headers
status: passed
---
---
type: pre_push_report
description: Results of running various checks prior to pushing changes.
report:
- task: run_javascript_examples
status: na
- task: run_c_examples
status: failed
---
---
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: na
- task: lint_package_json
status: na
- task: lint_repl_help
status: na
- task: lint_javascript_src
status: na
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: na
- task: lint_javascript_tests
status: na
- task: lint_javascript_benchmarks
status: na
- 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: na
- task: lint_typescript_tests
status: na
- task: lint_license_headers
status: passed
---
---
type: pre_push_report
description: Results of running various checks prior to pushing changes.
report:
- task: run_javascript_examples
status: na
- task: run_c_examples
status: na
- task: run_cpp_examples
status: na
- task: run_javascript_readme_examples
status: na
- task: run_c_benchmarks
status: na
- task: run_cpp_benchmarks
status: na
- task: run_fortran_benchmarks
status: na
- task: run_javascript_benchmarks
status: na
- task: run_julia_benchmarks
status: na
- task: run_python_benchmarks
status: na
- task: run_r_benchmarks
status: na
- task: run_javascript_tests
status: na
---
lapack/base/dlacpylapack/base/dlacpy
|
@kgryte Please review this pr if possible. thank you. |
kgryte
reviewed
Mar 15, 2025
Signed-off-by: Athan <kgryte@gmail.com>
kgryte
reviewed
Mar 15, 2025
| * @param strideB2 stride of the second dimension of `B` | ||
| * @param offsetB starting index for `B` | ||
| */ | ||
| void API_SUFFIX(c_copy_all)( const CBLAS_INT M, const CBLAS_INT N, const double *A, const CBLAS_INT strideA1, const CBLAS_INT strideA2, const CBLAS_INT offsetA, double *B, const CBLAS_INT strideB1, const CBLAS_INT strideB2, const CBLAS_INT offsetB ) { |
There was a problem hiding this comment.
These should be private functions. Right now, you are exposing them outside the translation unit. That is not desired.
kgryte
reviewed
Mar 15, 2025
| * @param strideB2 stride of the second dimension of `B` | ||
| * @param offsetB starting index for `B` | ||
| */ | ||
| void API_SUFFIX(c_copy_upper)( const CBLAS_INT M, const CBLAS_INT N, const double *A, const CBLAS_INT strideA1, const CBLAS_INT strideA2, const CBLAS_INT offsetA, double *B, const CBLAS_INT strideB1, const CBLAS_INT strideB2, const CBLAS_INT offsetB ) { |
kgryte
reviewed
Mar 15, 2025
| * @param strideB2 stride of the second dimension of `B` | ||
| * @param offsetB starting index for `B` | ||
| */ | ||
| void API_SUFFIX(c_copy_lower)( const CBLAS_INT M, const CBLAS_INT N, const double *A, const CBLAS_INT strideA1, const CBLAS_INT strideA2, const CBLAS_INT offsetA, double *B, const CBLAS_INT strideB1, const CBLAS_INT strideB2, const CBLAS_INT offsetB ) { |
kgryte
reviewed
Mar 15, 2025
| } | ||
|
|
||
| for ( i1 = 0; i1 < N; i1++ ) { | ||
| for ( i0 = 0; i0 <= stdlib_base_fast_min( i1, M-1 ); i0++ ) { |
There was a problem hiding this comment.
You shouldn't be using fast_min. That is for doubles. Instead, either define an internal function or a macro for returning the min of two integers.
kgryte
reviewed
Mar 15, 2025
| ib = offsetB; | ||
| if ( stdlib_ndarray_is_row_major( 2, strides ) ) { | ||
| for ( i1 = 0; i1 < M; i1++ ) { | ||
| for ( i0 = 0; i0 <= stdlib_base_fast_min( i1, N-1 ); i0++ ) { |
kgryte
reviewed
Mar 15, 2025
| #include "stdlib/lapack/base/dlacpy.h" | ||
| #include "stdlib/blas/base/shared.h" | ||
| #include "stdlib/strided/base/stride2offset.h" | ||
| #include "stdlib/math/base/special/fast/min.h" |
There was a problem hiding this comment.
You shouldn't be using this API.
kgryte
reviewed
Mar 15, 2025
| sb2 = LDB; | ||
| } else { // order === 'row-major' | ||
| if ( LDA < N ) { | ||
| return; |
There was a problem hiding this comment.
This is wrong. You should be using xerbla. The JS implementation raises exceptions. That follows BLAS/LAPACK. We should do the same here.
kgryte
reviewed
Mar 15, 2025
| /** | ||
| * Copies all or part of a matrix `A` to another matrix `B` using alternative indexing semantics. | ||
| * | ||
| * @private |
There was a problem hiding this comment.
Suggested change
| * @private |
Why is this annotation here?
kgryte
reviewed
Mar 15, 2025
| * @param M number of rows in matrix `A` | ||
| * @param N number of columns in matrix `A` | ||
| * @param A input matrix | ||
| * @param strideA1 stride of the first dimension of `A` |
There was a problem hiding this comment.
Prefer at least two spaces between a parameter and its description. Will require re-aligning all descriptions.
kgryte
reviewed
Mar 15, 2025
| */ | ||
| void API_SUFFIX(c_dlacpy_ndarray)( const CBLAS_UPLO uplo, const CBLAS_INT M, const CBLAS_INT N, const double *A, const CBLAS_INT strideA1, const CBLAS_INT strideA2, const CBLAS_INT offsetA, double *B, const CBLAS_INT strideB1, const CBLAS_INT strideB2, const CBLAS_INT offsetB ) { | ||
| if ( uplo == CblasUpper ) { | ||
| c_copy_upper( M, N, A, strideA1, strideA2, offsetA, B, strideB1, strideB2, offsetB ); |
There was a problem hiding this comment.
If you are wrapping c_copy_upper in API_SUFFIX, you need to do the same here. Applies below, as well.
kgryte
reviewed
Mar 15, 2025
| STDLIB_NAPI_ARGV_INT64( env, LDA, argv, 5 ); | ||
| STDLIB_NAPI_ARGV_INT64( env, LDB, argv, 7 ); | ||
|
|
||
| STDLIB_NAPI_ARGV_STRIDED_FLOAT64ARRAY( env, A, M*N, 1, argv, 4 ); |
There was a problem hiding this comment.
This is wrong. You need to be using argv_strided_float64array2d. Applies here and below.
kgryte
reviewed
Mar 15, 2025
| STDLIB_NAPI_ARGV_INT64( env, strideB2, argv, 9 ); | ||
| STDLIB_NAPI_ARGV_INT64( env, offsetB, argv, 10 ); | ||
|
|
||
| STDLIB_NAPI_ARGV_STRIDED_FLOAT64ARRAY( env, A, M*N, 1, argv, 3 ); |
kgryte
reviewed
Mar 15, 2025
| // Specify the number of elements along each dimension of `A`: | ||
| const int N = 3; | ||
|
|
||
| // Copies all or part of a matrix `A` to another matrix `B`: |
There was a problem hiding this comment.
Suggested change
| // Copies all or part of a matrix `A` to another matrix `B`: | |
| // Copy the upper triangular part of a matrix `A` to another matrix `B`: |
kgryte
reviewed
Mar 15, 2025
| printf( "B[ %i ] = %f\n", i, B[ i ] ); | ||
| } | ||
|
|
||
| // Copies all or part of a matrix `A` to another matrix `B` using alternative indexing semantics: |
There was a problem hiding this comment.
Suggested change
| // Copies all or part of a matrix `A` to another matrix `B` using alternative indexing semantics: | |
| // Copy the lower triangular part of a matrix `A` to another matrix `B` using alternative indexing semantics: |
kgryte
reviewed
Mar 15, 2025
| TODO | ||
| The function accepts the following arguments: | ||
|
|
||
| - **order**: `[in] CBLAS_LAYOUT` storage layout of `A` and `B` |
There was a problem hiding this comment.
All these descriptions are missing ending periods.
kgryte
reviewed
Mar 15, 2025
| TODO | ||
| #include "stdlib/blas/base/shared.h" | ||
|
|
||
| double A[] = { 1.0, 2.0, 3.0, 4.0 }; |
There was a problem hiding this comment.
Suggested change
| double A[] = { 1.0, 2.0, 3.0, 4.0 }; | |
| const double A[] = { 1.0, 2.0, 3.0, 4.0 }; |
This array is not mutated. Applies here and below.
kgryte
reviewed
Mar 15, 2025
| ```c | ||
| #include "stdlib/blas/base/shared.h" | ||
|
|
||
| double A[] = { 1.0, 2.0, 3.0, 4.0, 5.0 }; |
There was a problem hiding this comment.
Why do these arrays have an odd number of elements?
kgryte
reviewed
Mar 15, 2025
|
|
||
| The function accepts the following arguments: | ||
|
|
||
| - **uplo**: `[in] CBLAS_UPLO` specifies whether to copy the upper or lower triangular/trapezoidal part of matrix `A` |
kgryte
reviewed
Mar 15, 2025
Comment on lines
+300
to
+301
|
|
||
| return 0; |
There was a problem hiding this comment.
Suggested change
| return 0; |
This isn't needed.
kgryte
requested changes
Mar 15, 2025
|
@kgryte Thank you for your review. I will make sure to pay more attention to details as well as improve the quality of pr even more. As i open this pr way back it have some issues i will resolve them soon. |
|
@0PrashantYadav0 Are you planning on updating this PR or should we close this out? |
|
/stdlib merge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves None.
Description
This pull request:
dlacpyRelated Issues
This pull request:
Questions
No.
Other
No.
Checklist
@stdlib-js/reviewers