[stdlib_linalg] matrix property checks

[ghbrown]

As discussed in #482 and #476, this adds common matrix property checks (all returning a logical), including:

• is_square(A)
• is_diagonal(A), extended for nonsquare matrices as well
• is_symmetric(A)
• is_skew_symmetric(A)
• is_hermitian(A)
• is_triangular(A,uplo), input uplo in {'u',U','l','L'} to decide if checking for upper or lower, also extended to nonsquare matrices
• is_hessenberg(A,uplo), input uplo in {'u',U','l','L'} to decide if checking for upper or lower, also extended to nonsquare matrices

cmake and make builds appear to go fine (with a few warnings). Testing via cmake also gives quite a few warnings specifically related to the relevant tests, but all pass with no problem. The tests are especially long (~2000 lines worth), but I've discussed assisting in porting them over to something friendlier (fypp / test-drive/ etc.) after this PR is finished.

Documentation has not been written yet, I wanted to check in midway and make sure everything looked acceptable. Thanks.

[ghbrown]

I just realized the tests for is_hermitian are missing completely, I will address this.

[ghbrown]

The most recent commit says is_hamiltonian which is clearly incorrect and meant to be is_hermitian, but I will not change it in the interest of not force pushing.

[ghbrown]

I believe with the docs added all of the major tasks on my side are complete.

I was getting an error from one of the test builds, but it appears I have now fixed that. Please let me know if this is not the case.

[arjenmarkus]

Hermiticity comes from the name of the French mathematician Charles Hermite, who invented the concept. Hermeticity comes from the Greek god Hermes, who in Renaissance times and earlier was also known as Hermes Trismegistus and in that guise he was the god/patron "saint" of the alchemists. So if Iwere you, I'd opt for the first spelling ;), even though the second is more common in everyday speech. Op ma 27 sep. 2021 om 16:36 schreef Gabriel Brown ***@***.***

…

: ***@***.**** commented on this pull request. ------------------------------ In src/stdlib_linalg.fypp <#499 (comment)>: > + logical :: res + ${t1}$ :: zero + integer :: m, n, o, i, j + zero = 0 !zero of relevant type + m = size(A,1) + n = size(A,2) + do j = 1, n !loop over all columns + o = min(j-1,m) !index of row above diagonal (or last row) + do i = 1, o !loop over rows above diagonal + if (.not. (A(i,j) .eq. zero)) then + res = .false. + return + end if + end do + do i = o+2, m !loop over rows below diagonal + if (.not. (A(i,j) .eq. zero)) then No good reason. I always forget which language use which negated equality with symbols (between !=, ~=, /=, and more), so I just reverted to something I knew would work (while forgetting about .neq.). I'll make the changes, thanks for catching this. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#499 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN6YR6EJ4UOAHIZP7V7UBTUEB6NHANCNFSM5C6Q5BDA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[ghbrown]

@arjenmarkus Thanks for the etymology!
Yes, the use of "Hermeticity" here was never in question, I just brought it up as a possible point against the use of the very similar "Hermiticity". Our decision is between "Hermiticity" and "Hermicity", which I believe both stem from Hermite.

[milancurcic]

Thanks! I left a number of specific comments on style, namely on using the modern relational operators instead of the old ones (still standard, but deprecated by Metcalf, Reid & Cohen).

[ghbrown]

@milancurcic Thanks for the style notes. I always preferred the alphabetic relational operators since they read easily (like Python), but did not realize they had been deprecated (too bad). I will update to conform.
As for the redundant parentheses, I prefer them as they help keep "thoughts" together which can be hard when you have expressions like a == b .or. c == d instead of (a == b) .or. (c == d) (which is much more transparent to me).
However, your suggestion of using to_lower() largely avoids the need for such parentheses, and I think it's a much more elegant approach.
Thanks for the review, I'll get on these changes.

[milancurcic]

@ghbrown FWIW here's how I've been doing it, there may be other or better ways:

1. Have a local repo cloned from your fork of stdlib, at the matrix_property_checks branch
2. Add fortran-lang/stdlib as a remote repo (git add remote upstream https://github.com/fortran-lang/stdlib)
3. git fetch upstream
4. git merge upstream/master into matrix_property_checks
5. Resolve conflicts manually in a text editor
6. Commit changes to matrix_property_checks and push

[jvdp1]

@ghbrown FWIW here's how I've been doing it, there may be other or better ways:

1. Have a local repo cloned from your fork of stdlib, at the matrix_property_checks branch
2. Add fortran-lang/stdlib as a remote repo (git add remote upstream https://github.com/fortran-lang/stdlib)
3. git fetch upstream
4. git merge upstream/master into matrix_property_checks
5. Resolve conflicts manually in a text editor
6. Commit changes to matrix_property_checks and push

@ghbrown Note the conflict with test_linalg.f90 is due to the fact that test_linalg.f90 has bee renamed test_linalg.fypp and that it now supports the testdrive procedure. Let us know if you need help with this too.

[ghbrown]

@milancurcic Thanks! I had been tracking it via the upstream remote, but still haven't been able to materialize any merge conflicts locally.
I think my trouble is in step 4. Are you suggesting

git checkout matrix_property_checks
git merge upstream/master

will alert me of the relevant merge conflicts?

@jvdp1 Thanks for the heads up on that. I had originally wanted to use fypp anyway since the f90 version was quite large, so that's good. I'll take a look at fortran-lang/testdrive and find existing examples in the standard library, but if I get stuck I'll reach out.

[jvdp1]

@milancurcic Thanks! I had been tracking it via the upstream remote, but still haven't been able to materialize any merge conflicts locally. I think my trouble is in step 4. Are you suggesting

git checkout matrix_property_checks
git merge upstream/master

will alert me of the relevant merge conflicts?

Yes, it will. I just tested it and the two conflicts mentioned on this page are mentioned too.

@jvdp1 Thanks for the heads up on that. I had originally wanted to use fypp anyway since the f90 version was quite large, so that's good. I'll take a look at fortran-lang/testdrive and find existing examples in the standard library, but if I get stuck I'll reach out.

The current test_linalg.fypp already contains testdrive procedures. You "just" need to transfer yours to support it.

[jvdp1]

@ghbrown did you get a chance to look at this PR? Don't hesitate to ping me if you need help.

[ghbrown]

@jvdp1 I took a quick look about a week ago when it was bumped and have a general idea of what needs to be done and slightly less of an idea of how to do it.

The real constraint for me is time, of which I'll have almost none until December 20. If it can wait that long, I'm more than happy to take care of it, as I think it would be good to see what's new with testdrive, etc. I suspect I'll be able to figure things out just fine when I actually have time to work on it, but if not I will reach out. Apologies for the delay.

[jvdp1]

@ghbrown Thank you for answer. No worries. There is no rush!

[ghbrown]

A bit later than planned, but with the latest commits things are hopefully much closer to completion.
Merge conflicts have been addressed and I have integrated the tests for the features using testdrive while taking advantage of fypp as much as possible.

A few outstanding concerns:

• The (ubuntu-latest, 10, make) test is failing on the manual makefiles. I assume this is a problem with my branch, but I haven't been able to extract much from the details of the failing check.
• Although it builds and tests and checks mostly okay, I'm not convinced the tests I implemented are actually running properly (or at all), since even when I attempt to make them fail intentionally, I still get 100% passing. My guess is that I've done something wrong with testdrive, but I'm not sure.

Any suggestions on these concerns would be greatly appreciated.

[awvwgk]

The manual makefile build probably fails because of a missing module dependency (those have to be added by hand, because make doesn't know how to compile Fortran without our help). Missing dependencies might or might not show up as build failure, we try to run make with as many threads as possible in the CI to ensure it almost always fails when a dependency is not specified.

Regarding test-drive, you can always run the test executable from the build directory directly to inspect the printout manually. Calling check(error, .false.) twice should make the test fail for sure in CTest (second call escalates the error in final procedure).

I can also have a look on those issues, if you like.

[ghbrown]

@awvwgk Thanks for the suggestions. The issue with tests not failing when intended was a mistake in my fypp, and the manual makefile issue was a remnant of the merge conflict fixes, so both of my concerns should be addressed and fixed, with all tests passing.

A lot has changed since this PR originally got its approvals, so if further review is necessary it's no problem.

[milancurcic]

Thanks @ghbrown and reviewers, let's merge tomorrow if there are no further objections.

[jvdp1]

Before merging, could you remove the files related to the hash procedures, please? They should not appear in this PR.
Otherwise, I have no other comments. Thank you @ghbrown

[ghbrown]

@jvdp1 Thanks for catching that, fixed.

Should those get added to a .gitignore at some point?

[jvdp1]

@jvdp1 Thanks for catching that, fixed.

Should those get added to a .gitignore at some point?

yes, I think it should be. But not in this PR. Could you open a new PR with those added to .gitignore, please?

[jvdp1]

Thank you @ghbrown for this PR, and also all reviewers. I will merge it.

[ghbrown]

yes, I think it should be. But not in this PR. Could you open a new PR with those added to .gitignore, please?

@jvdp1 Sure, no problem.