Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

linalg: determinant #798

Merged
merged 29 commits into from
Apr 25, 2024
Merged

linalg: determinant #798

merged 29 commits into from
Apr 25, 2024

Conversation

perazz
Copy link
Contributor

@perazz perazz commented Apr 13, 2024

Introduce determinant operator, based on LAPACK *GETRF functions.

  • implementation
  • tests
  • exclude unsupported xdp
  • documentation
  • submodule

Prior art:

  • Numpy: det(a)
  • Scipy: det(a, overwrite_a=False, check_finite=True)
  • IMSL: DET(a)

Proposed implementation: generic interface

  1. d = det(a) -> pure function, no additional inputs, a is unchanged
  2. d = det(a,overwrite_a=.false.,err=state) -> not pure, option to overwrite_a (do not allocate a temporary), return error state handler
  3. .det.a Fortran operator interface, similar to 1), pure, but with the operator method

Error handling returns:

  • LINALG_ERROR on singular matrix
  • LINALG_VALUE_ERROR on invalid matrix size

cc @jvdp1 @jalvesz @fortran-lang/stdlib

@@ -0,0 +1,302 @@
#:include "common.fypp"
#:set RC_KINDS_TYPES = REAL_KINDS_TYPES + CMPLX_KINDS_TYPES
module stdlib_linalg_determinant
Copy link
Member

Choose a reason for hiding this comment

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

Could it be a submdodule of stdblib_linalg, such that users can use it as:

use stdlib_linalg, only: det

instead of:

use stdlib_linalg_determinant, only: det

This will avoid an enormous amount of stdlib_linalg modules.
Inconvenient: this approach will require the compilation of the a large module.

What do you think? What would be the best strategy for the users?

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a good approach, everything would fit together smoothly. This might require indeed a large header module but for the end user and even a developer who would like to pick just one method and work on it, it would be easier... I think.

@perazz
Copy link
Contributor Author

perazz commented Apr 14, 2024

Thanks @jvdp1 @jalvesz, great to iterate on the format, the more time we spend on this first LAPACK-backed function, the easier will be later.
The submodule one is a great idea and here is an implementation.

gfortran returns an error though:

Fixed thanks to Fortran Discourse

Copy link
Member

@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

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

Thank you @perazz . Here are some suggestions. Overall, I like the API.

Comment on lines +30 to +32

! Export linalg error handling
public :: linalg_state_type, linalg_error_handling
Copy link
Member

Choose a reason for hiding this comment

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

This let me think that we should review the structure of the specs. Currently we have 1 page per module. However, if these become available through stdlib_linalg, the specs should be modified accordingly (probably in another PR; I can start to work on a proposition when this is ready).

src/stdlib_linalg.fypp Outdated Show resolved Hide resolved
src/stdlib_linalg.fypp Outdated Show resolved Hide resolved
src/stdlib_linalg.fypp Show resolved Hide resolved
src/stdlib_linalg.fypp Show resolved Hide resolved
test/linalg/test_linalg_determinant.fypp Show resolved Hide resolved
!!```
!!
#:for rk,rt in RC_KINDS_TYPES
#:if rk!="xdp"
Copy link
Member

Choose a reason for hiding this comment

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

For future developers, it might be good to add a comment to explain why xdp is excluded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@perazz perazz Apr 15, 2024

Choose a reason for hiding this comment

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

Even though it is not automated generation, should we manually implement blas and lapack backends for xdp? They would be the same as for quad-precision, because all constants are generalized, and 64-bit double precision is still the "lower precision" datatype for mixed-precision algorithms. We should just decide about real/complex prefixes, like s/c vs. d/z vs. q/w, for xdp they may be x/y?

Copy link
Member

Choose a reason for hiding this comment

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

Supporting xdp would be a nice feature IMO (even if I never use xdp). x/y prefixes make sense to me. If you think it is easy to implement it, I would support it. However, I don't think it is a priority right now.

Copy link
Contributor

@jalvesz jalvesz Apr 15, 2024

Choose a reason for hiding this comment

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

If it serves for the discussion, I'm using the following:
in the common.fypp I added the notion of SUFFIX like

...
#! Collected (kind, type, suffix) tuples for real types
#:set REAL_KINDS_TYPES = list(zip(REAL_KINDS, REAL_TYPES, REAL_KINDS))

#! Complex types to be considered during templating
#:set CMPLX_SUFFIX = ["c{}".format(k) for k in CMPLX_KINDS]

#! Collected (kind, type, suffix) tuples for complex types
#:set CMPLX_KINDS_TYPES = list(zip(CMPLX_KINDS, CMPLX_TYPES, CMPLX_SUFFIX))

Which then allows me to do this in the .fypp

#:for k1, t1, s1 in (KINDS_TYPES)
    type, public, extends(COO_t) :: COO_${s1}$
    ${t1}$, allocatable :: data(:) 
contains
     procedure :: get => get_value_coo_${s1}$
     procedure :: set => set_value_coo_${s1}$
end type
#:endfor

and obtaining this in the fortran file:

    type, public, extends(COO_t) :: COO_sp
        real(sp), allocatable :: data(:) 
    contains
        procedure :: get => get_value_coo_sp
        procedure :: set => set_value_coo_sp
    end type
    type, public, extends(COO_t) :: COO_dp
        real(dp), allocatable :: data(:) 
    contains
        procedure :: get => get_value_coo_dp
        procedure :: set => set_value_coo_dp
    end type
    type, public, extends(COO_t) :: COO_csp
        complex(sp), allocatable :: data(:) 
    contains
        procedure :: get => get_value_coo_csp
        procedure :: set => set_value_coo_csp
    end type
    type, public, extends(COO_t) :: COO_cdp
        complex(dp), allocatable :: data(:) 
    contains
        procedure :: get => get_value_coo_cdp
        procedure :: set => set_value_coo_cdp
    end type

So basically considering the real types as reference, and the complex types as extensions adding the c in front of the suffix.

src/stdlib_linalg_determinant.fypp Outdated Show resolved Hide resolved
src/stdlib_linalg_determinant.fypp Outdated Show resolved Hide resolved
src/stdlib_linalg_determinant.fypp Outdated Show resolved Hide resolved
Copy link
Member

@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

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

Thank you @perazz . This PR is almost ready to be merged IMO. Specs should be still added.

src/stdlib_linalg.fypp Show resolved Hide resolved
src/stdlib_linalg.fypp Show resolved Hide resolved
Copy link
Member

@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

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

Thank you @perazz. The specs LGTM. I proposed some minor suggestions.

doc/specs/stdlib_linalg.md Outdated Show resolved Hide resolved
doc/specs/stdlib_linalg.md Show resolved Hide resolved
doc/specs/stdlib_linalg.md Outdated Show resolved Hide resolved
doc/specs/stdlib_linalg.md Outdated Show resolved Hide resolved
doc/specs/stdlib_linalg.md Outdated Show resolved Hide resolved
doc/specs/stdlib_linalg.md Show resolved Hide resolved
doc/specs/stdlib_linalg.md Outdated Show resolved Hide resolved
doc/specs/stdlib_linalg.md Outdated Show resolved Hide resolved
perazz and others added 6 commits April 21, 2024 16:17
Co-authored-by: Jeremie Vandenplas <[email protected]>
Co-authored-by: Jeremie Vandenplas <[email protected]>
Co-authored-by: Jeremie Vandenplas <[email protected]>
Co-authored-by: Jeremie Vandenplas <[email protected]>
Co-authored-by: Jeremie Vandenplas <[email protected]>
Co-authored-by: Jeremie Vandenplas <[email protected]>
@perazz
Copy link
Contributor Author

perazz commented Apr 21, 2024

Thanks a lot @jvdp1 - your edits look good! I've just added that it works for either real or complex matrices (it would seem it was for real only).

@perazz
Copy link
Contributor Author

perazz commented Apr 23, 2024

Thank you @jvdp1 @jalvesz for the reviews, let's wait a bit more, and if there are no further comments, I believe we could merge this before the end of the week.

@jvdp1
Copy link
Member

jvdp1 commented Apr 25, 2024

I'll merge this PR. Thank you @perazz for this nice feature.

@jvdp1 jvdp1 merged commit 28ae6e0 into fortran-lang:master Apr 25, 2024
17 checks passed
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