-
Notifications
You must be signed in to change notification settings - Fork 197
[stdlib_linalg] matrix property checks #499
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
Conversation
I just realized the tests for is_hermitian are missing completely, I will address this.
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.
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 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
@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.
@ghbrown FWIW here's how I've been doing it, there may be other or better ways:
- Have a local repo cloned from your fork of stdlib, at the matrix_property_checks branch
- Add fortran-lang/stdlib as a remote repo (
git add remote upstream https://github.com/fortran-lang/stdlib) git fetch upstreamgit merge upstream/masterinto matrix_property_checks- Resolve conflicts manually in a text editor
- Commit changes to matrix_property_checks and push
@ghbrown FWIW here's how I've been doing it, there may be other or better ways:
- Have a local repo cloned from your fork of stdlib, at the matrix_property_checks branch
- Add fortran-lang/stdlib as a remote repo (
git add remote upstream https://github.com/fortran-lang/stdlib)git fetch upstreamgit merge upstream/masterinto matrix_property_checks- Resolve conflicts manually in a text editor
- 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.
@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.
@milancurcic Thanks! I had been tracking it via the
upstreamremote, but still haven't been able to materialize any merge conflicts locally. I think my trouble is in step 4. Are you suggestinggit checkout matrix_property_checks git merge upstream/masterwill 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
fyppanyway since thef90version was quite large, so that's good. I'll take a look atfortran-lang/testdriveand 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.
@ghbrown did you get a chance to look at this PR? Don't hesitate to ping me if you need help.
@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.
@ghbrown Thank you for answer. No worries. There is no rush!
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.
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.
@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.
Thanks @ghbrown and reviewers, let's merge tomorrow if there are no further objections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@jvdp1 Thanks for catching that, fixed.
Should those get added to a .gitignore at some point?
@jvdp1 Thanks for catching that, fixed.
Should those get added to a
.gitignoreat 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?
Thank you @ghbrown for this PR, and also all reviewers. I will merge it.
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.
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 wellis_symmetric(A)is_skew_symmetric(A)is_hermitian(A)is_triangular(A,uplo), inputuploin{'u',U','l','L'}to decide if checking for upper or lower, also extended to nonsquare matricesis_hessenberg(A,uplo), inputuploin{'u',U','l','L'}to decide if checking for upper or lower, also extended to nonsquare matricescmakeandmakebuilds appear to go fine (with a few warnings). Testing viacmakealso 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.