Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
jvdp1 merged 37 commits into fortran-lang:master from ghbrown:matrix_property_checks
Dec 31, 2021

Conversation

@ghbrown
Copy link
Member

@ghbrown ghbrown commented Aug 28, 2021

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.

zoziha reacted with thumbs up emoji
@awvwgk awvwgk added the reviewers needed This patch requires extra eyes label Aug 28, 2021
Copy link
Member Author

ghbrown commented Aug 30, 2021

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

Copy link
Member Author

ghbrown commented Sep 2, 2021

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.

Copy link
Member Author

ghbrown commented Sep 6, 2021
edited
Loading

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.

@awvwgk awvwgk added the topic: mathematics linear algebra, sparse matrices, special functions, FFT, random numbers, statistics, ... label Sep 18, 2021
Copy link
Member

arjenmarkus commented Sep 27, 2021 via email

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

Copy link
Member Author

ghbrown commented Sep 27, 2021

@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.

Copy link
Member

@milancurcic milancurcic left a 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).

Copy link
Member Author

ghbrown commented Sep 27, 2021

@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 and 14NGiestas reacted with thumbs up emoji

Copy link
Member

@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

Copy link
Member

jvdp1 commented Nov 27, 2021

@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.

Copy link
Member Author

ghbrown commented Nov 27, 2021

@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.

Copy link
Member

jvdp1 commented Nov 27, 2021

@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.

ghbrown reacted with thumbs up emoji

Copy link
Member

jvdp1 commented Dec 7, 2021

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

@jvdp1 jvdp1 added the waiting for OP This patch requires action from the OP label Dec 7, 2021
Copy link
Member Author

ghbrown commented Dec 8, 2021

@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.

Copy link
Member

jvdp1 commented Dec 8, 2021

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

Copy link
Member Author

ghbrown commented Dec 27, 2021

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.

Copy link
Member

awvwgk commented Dec 27, 2021

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.

Copy link
Member Author

ghbrown commented Dec 28, 2021

@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.

Copy link
Member

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

ghbrown reacted with thumbs up emoji awvwgk and zoziha reacted with rocket emoji

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.

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

Copy link
Member Author

ghbrown commented Dec 31, 2021

@jvdp1 Thanks for catching that, fixed.

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

Copy link
Member

jvdp1 commented Dec 31, 2021

@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?

Copy link
Member

jvdp1 commented Dec 31, 2021

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

awvwgk and ghbrown reacted with rocket emoji

@jvdp1 jvdp1 merged commit 2b60648 into fortran-lang:master Dec 31, 2021
Copy link
Member Author

ghbrown commented Dec 31, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@milancurcic milancurcic milancurcic approved these changes

@ivan-pi ivan-pi ivan-pi approved these changes

@jvdp1 jvdp1 jvdp1 approved these changes

@awvwgk awvwgk awvwgk approved these changes

Assignees

No one assigned

Labels

topic: mathematics linear algebra, sparse matrices, special functions, FFT, random numbers, statistics, ... waiting for OP This patch requires action from the OP

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /