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.

SRC/dgecx.f Outdated
*> 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]

SRC/dgecx.f Outdated
*> 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.

SRC/dgecx.f Outdated
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.

SRC/dgecx.f Outdated
*
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.

SRC/dgecx.f Outdated
* 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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, C is literally a subset of A.

*>
*> 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

@mgates3
Copy link
Collaborator

mgates3 commented Nov 5, 2025

We've discussed that this is the expert interface, and we would have a simplified interface (e.g., without all the column selection). Elsewhere in LAPACK, expert interfaces have an extra x, such as gesvx, gesvxx, posvx, posvxx. Should we rename this one gecxx and create the simplified interface as gecx?

* http://www.netlib.org/lapack/explore-html/
*
*> \htmlonly
*> Download DGEQP3RK + dependencies
Copy link
Collaborator

Choose a reason for hiding this comment

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

GECXX instead of GEQP3RK.

*> \author Univ. of Colorado Denver
*> \author NAG Ltd.
*
*> \ingroup gecxx
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to add to DOCS/groups-usr.dox:

    @defgroup gecxx  gecxx: CX decomposition

We may need a new category. CX doesn't seem to fit in existing categories.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Jim suggests a category "Low rank factorizations". Given that, I suggest adding this, or a subset of this, to groups-user.dox between unitary_top @{ ... @} and geev_top @{ ... @} top-level groups (line 456):

    @defgroup low_rank_top          Low-rank factors (CX, CUR, etc.)
    @{
        @defgroup cx_grp            CX
        @{
            @defgroup gecx          gecx:           CX factor
            @defgroup gecxx         gecxx:          CX factor, expert interface
        @}

        @defgroup cur_grp           CUR
        @{
            @defgroup gecur         gecur:          CUR factor
            @defgroup gecurx        gecurx:         CUR factor, expert interface
        @}
    @}

*> On exit, if INFO >= 0, WORK(1) returns the optimal LIWORK.
*> \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.

\param[in]

NFREE = NSUB
*
LQUERY = ( LWORK.EQ.-1 )
LIQUERY = ( LIWORK.EQ.-1 )
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be simpler to have just one LQUERY variable. Cf. zheevd.f

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