Skip to content

Conversation

scr2016
Copy link
Contributor

@scr2016 scr2016 commented Oct 15, 2025

his a draft for dgecx.f. to REVIEW and COMMENT

@scr2016 scr2016 marked this pull request as draft October 15, 2025 07:09
Copy link
Collaborator

@mgates3 mgates3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial comments.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is both DGECX.f (incorrect) and dgecx.f (correct) file? This file should be removed.

*> Specifies how the factors of CX factorization
*> are returned.
*>
*> = 'P' or 'p' : return only the column permutaion matrix P
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pedantic: LAPACK doesn't normally specify both Uppercase and lowercase. Cf.:

lapack/SRC> grep "^\*> += '\w'" lapack/SRC/zpotrf.f
*>          = 'U':  Upper triangle of A is stored;
*>          = 'L':  Lower triangle of A is stored.

lapack/SRC> grep "^\*> += '\w'" lapack/SRC/*.f`

*> matrix in the blocked step auxiliary subroutine DLAQP3RK ).
*> \endverbatim
*>
*> \param[out] LIWORK
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Input, right? \param[in]

*> The dimension of the array LIWORK. LIWORK >= N
*>
*> If LIWORK = -1, then a workspace query is assumed; the routine
*> only calculates the optimal size of the WORK array, returns
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: IWORK array.

SRC/dgecx.f Outdated
*
* DGECX
*
END No newline at end of file
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Files should end with a newline. Notice ⊖ on Github.

NBOPT = ILAENV( INB, 'DGEQRF', ' ', M, N, -1, -1 )
LWKOPT = 1000
END IF
WORK( 1 ) = DBLE( LWKOPT )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add:
LWORK( 1 ) = N
or whatever the formula is. Also elsewhere.

*
END IF
*
WORK( 1 ) = DBLE( LWKOPT )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add:
LWORK( 1 ) = N
as above.

* MSUB.
*
INFO = -6
WORK( 1 ) = DBLE( LWKOPT )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add:
LWORK( 1 ) = N
or whatever the formula is. Also elsewhere.

@scr2016
Copy link
Contributor Author

scr2016 commented Oct 16, 2025 via email

changed the description of DGECX
*>
*> A * P(K) = C*X + A_resid, where
*>
*> C is an M-by-K matrix which is a subset of K columns selected
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think with "subset" you mean "spanning the same subspace" or "in the range of K columns of A" here. Is that right?

*>
*> DGECX computes a CX factorization of a real M-by-N matrix A:
*>
*> A * P(K) = C*X + A_resid, where
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think P(K) notation is confusing here. It's not a function of K but depends on K parameter.

*> from the original matrix A,
*>
*> X is a K-by-N matrix that minimizes the Frobenius norm of the
*> residual matrix A_resid, X = pseudoinv(C) * A,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just say "minimizes "|A - C*X|" in the Frobenius norm".

*> Column selection stage 1.
*> =========================
*>
*> The user can select N_sel columns and deselect N_desel columns
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't I just supply a list of columns that I want and leave this deselect business out? Then it will be two options, "A" is all columns, "S" selected columns. If "A" then cols array is not referenced otherwise it is read from cols array.

I'm afraid this is getting a bit too complicated with explicit deselection. Same goes for the rows

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.

3 participants