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

Kronecker Product addition to stdlib_linalg #700

Merged
merged 12 commits into from Mar 8, 2023

Conversation

adenchfi
Copy link
Contributor

@adenchfi adenchfi commented Mar 3, 2023

Based on this issue, (#699)
I decided to go ahead and make a PR with this functionality. In case I go forward with the GSoC program, I figured the sooner the better.

I basically used the same format as linalg_outer_product for the .fypp source and for the unit tests. I confirmed I can build the stdlib with these additions with cmake, and that all unit tests pass.

The API and ordering convention used is similar to numpy.kron, though my name is slightly longer. Additionally, because there is both a Kronecker product and a Kronecker sum, I decided to use the full name kronecker_product.

It was suggested to go small with the PRs, and this is my first one, so I have not included kronecker_sum in this PR.

PS: There is no change to the README, I am not sure why there was anything to commit in that one. I was adding an SSH key to my machine's git and I suppose (?) the commands it had me do forced another commit.

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 for this PR. I only have minor comments regarding the code.
Also, could you edit the specs and add this implementation, please?

src/stdlib_linalg.fypp Outdated Show resolved Hide resolved
src/stdlib_linalg_kronecker.fypp Outdated Show resolved Hide resolved
test/linalg/test_linalg.fypp Outdated Show resolved Hide resolved
test/linalg/test_linalg.fypp Outdated Show resolved Hide resolved
test/linalg/test_linalg.fypp Outdated Show resolved Hide resolved
test/linalg/test_linalg.fypp Outdated Show resolved Hide resolved
test/linalg/test_linalg.fypp Outdated Show resolved Hide resolved
src/stdlib_linalg_kronecker.fypp Outdated Show resolved Hide resolved
adenchfi and others added 4 commits March 4, 2023 15:36
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Adding spaces to = signs

Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
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.

Specs look good. Thank you.

doc/specs/stdlib_linalg.md Outdated Show resolved Hide resolved
example/linalg/example_kronecker_product.f90 Outdated Show resolved Hide resolved
example/linalg/example_kronecker_product.f90 Outdated Show resolved Hide resolved
example/linalg/example_kronecker_product.f90 Outdated Show resolved Hide resolved
adenchfi and others added 2 commits March 4, 2023 17:13
Adding spaces and capitalization.

Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
@14NGiestas 14NGiestas linked an issue Mar 4, 2023 that may be closed by this pull request
doc/specs/stdlib_linalg.md Outdated Show resolved Hide resolved
test/linalg/test_linalg.fypp Outdated Show resolved Hide resolved
adenchfi and others added 3 commits March 5, 2023 10:18
Fixing minor typo v -> B

Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Forgot to add the loop integers.

Co-authored-by: Ian Giestas Pauli <iangiestaspauli@gmail.com>
@adenchfi
Copy link
Contributor Author

adenchfi commented Mar 7, 2023

I believe there's nothing else for this particular PR?

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.

LGTM. I have no additional comments.
hank you for this PR.

@adenchfi
Copy link
Contributor Author

adenchfi commented Mar 8, 2023

LGTM. I have no additional comments. Thank you for this PR.

Thanks for guiding me through the process! I will work on the Kronecker sum as well sometime.

@adenchfi adenchfi closed this Mar 8, 2023
@adenchfi adenchfi reopened this Mar 8, 2023
@jvdp1
Copy link
Member

jvdp1 commented Mar 8, 2023

Thank you @adenchfi
@14NGiestas Can you have a final look, please? Then I will merge it!

@14NGiestas 14NGiestas closed this Mar 8, 2023
@14NGiestas 14NGiestas reopened this Mar 8, 2023
@14NGiestas
Copy link
Member

@jvdp1 LGTM - I'm just wondering if it works with arbitrarily indexed matrices (as an edge case for tests).

@jvdp1
Copy link
Member

jvdp1 commented Mar 8, 2023

@jvdp1 LGTM - I'm just wondering if it works with arbitrarily indexed matrices (as an edge case for tests).

I am not sure what you mean. Somehting like:

real :: mat(-5:7, 9:11)

@14NGiestas
Copy link
Member

14NGiestas commented Mar 8, 2023

@jvdp1 LGTM - I'm just wondering if it works with arbitrarily indexed matrices (as an edge case for tests).

I am not sure what you mean. Somehting like:

real :: mat(-5:7, 9:11)

That was what I had in mind (just to be picky), but I tested and it works anyway:

program main
    real :: A(0:2) = [0, 1, 2] ! or A(-1:1) and so on
    call m(A)
contains
    subroutine m(A)
        real :: A(:)
        print*, lbound(A), ubound(A) ! 1 3
    end subroutine
end program

@jvdp1
Copy link
Member

jvdp1 commented Mar 8, 2023

@jvdp1 LGTM - I'm just wondering if it works with arbitrarily indexed matrices (as an edge case for tests).

I am not sure what you mean. Somehting like:

real :: mat(-5:7, 9:11)

That was what I had in mind (just to be picky), but I tested and it works anyway:

program main
    real :: A(0:2) = [0, 1, 2] ! or A(-1:1) and so on
    call m(A)
contains
    subroutine m(A)
        real :: A(:)
        print*, lbound(A), ubound(A) ! 1 3
    end subroutine
end program

Thank you. I'll commit your last change and merge it.
Tahnk @adenchfi for your contributions

Co-authored-by: Ian Giestas Pauli <iangiestaspauli@gmail.com>
@jvdp1 jvdp1 merged commit 2fdfab4 into fortran-lang:master Mar 8, 2023
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.

Kronecker Product & Sum
3 participants