-
Notifications
You must be signed in to change notification settings - Fork 166
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
Provide state/error handling for linear algebra #774
Conversation
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.
I think we need a mechanism like this, thanks for creating it.
We just need to discuss the details of the implementation. I left some comments.
Another question to decide is whether this should be specialized to linear algebra, or made more general.
src/stdlib_linalg_state.fypp
Outdated
|
||
!> Error creation message, from N input variables (numeric or strings) | ||
pure type(linalg_state) function new_state_nowhere(flag,a1,a2,a3,a4,a5,a6,a7,a8,a9,a10, & | ||
v1,v2,v3,v4,v5) result(new_state) |
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.
I am not sure about this.
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.
Here's an example of how these functions allow easy coding of the error/state variable, for example for a complex number:
stdlib/test/linalg/test_linalg.fypp
Line 962 in c5c568b
state = linalg_state(LINALG_SUCCESS,' 32-bit array: ',v1=[(1.0_sp,0.0_sp),(0.0_sp,1.0_sp)]) |
And the expected user error message:
stdlib/test/linalg/test_linalg.fypp
Line 964 in c5c568b
' 32-bit array: [(1.00000000E+00,0.00000000E+00) (0.00000000E+00,1.00000000E+00)]', & |
The real formats for 32/64/128 bit are taken from here:
Lines 28 to 46 in 8853ecd
character(*), parameter :: & | |
!> Format string for integers | |
FMT_INT = '(i0)', & | |
!> Format string for single precision real numbers | |
FMT_REAL_SP = '(es15.8e2)', & | |
!> Format string for souble precision real numbers | |
FMT_REAL_DP = '(es24.16e3)', & | |
!> Format string for extended double precision real numbers | |
FMT_REAL_XDP = '(es26.18e3)', & | |
!> Format string for quadruple precision real numbers | |
FMT_REAL_QP = '(es44.35e4)', & | |
!> Format string for single precision complex numbers | |
FMT_COMPLEX_SP = '(es15.8e2,1x,es15.8e2)', & | |
!> Format string for double precision complex numbers | |
FMT_COMPLEX_DP = '(es24.16e3,1x,es24.16e3)', & | |
!> Format string for extended double precision complex numbers | |
FMT_COMPLEX_XDP = '(es26.18e3,1x,es26.18e3)', & | |
!> Format string for quadruple precision complex numbers | |
FMT_COMPLEX_QP = '(es44.35e4,1x,es44.35e4)' |
src/stdlib_linalg_state.fypp
Outdated
call appendv(new_state%message,v2) | ||
call appendv(new_state%message,v3) | ||
call appendv(new_state%message,v4) | ||
call appendv(new_state%message,v5) |
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.
And this mechanism.
end function new_state_nowhere | ||
|
||
! Append a generic value to the error flag | ||
pure subroutine append(msg,a,prefix) |
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.
Is this function appending a string representation of an integer, real or an array into the string message?
If so, I would keep it separate (the linalg library can append what it needs into the error message), not tie this into the implementation of the error state.
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.
Exactly @certik, it covers all real
, integer
, complex
kinds covered by stdlib (according to the preprocessing) and strings. I designed it this way so that error handling is easy to be read by the user but also easy to implement. See for example in my current solve
implementation:
I find it very easy to read - and no need to write format
s for each possible error string. Also, there are at least 5-6 possible error messages out of each LAPACK routine, it could be a format
nightmare down the road.
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.
One option could be to move the numeric->string routines to stdlib_io
?
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.
Could you use the stdlib
to_string
?
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.
Totally, although here I purposedly avoided allocatable
components, because there is going to be one linalg_state_type
variable in every function, and I wanted the compiler to be able to put it on the stack to minimize performance penalty. What do you think?
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.
Is performance a real problem here? I mean, if it is at this stage, it means that there is an issue.
Anyway, I am ok with the code as it is now. I am just wondering what would be the impact performance-wise.
Thanks a lot @certik for the review! We briefly touched this point during the last monthly call. It seems like the consensus was: let's try to use this error handling method for the linear algebra part first. If we see that we like it, don't find issues, etc., then it could be promoted to a global error handling paradigm for the whole stdlib/src/stdlib_linalg_state.fypp Lines 49 to 56 in c5c568b
|
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.
Thank you @perazz . Overall LGTM. Here are some suggestions.
It would be good to add some specs too.
src/stdlib_linalg_constants.fypp
Outdated
module stdlib_linalg_constants | ||
use stdlib_kinds, only: sp, dp, qp, int32, int64, lk | ||
use, intrinsic :: ieee_arithmetic, only: ieee_is_nan | ||
!$ use omp_lib |
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.
Is omp_lib
needed here?
!$ use omp_lib |
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.
Yes, it is used by the LAPACK functions. I put it in the constants module to avoid repeated use
statements.
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
address style changes
@certik @jvdp1 (@gnikit @jalvesz) do you have further comments that could help get this merged soon? Thanks @jvdp1's review, IMHO everything looks in line with stdlib's style now. After we have error/state handling merged, we can begin adding several high-level APIs in parallel, so people will have time to look and comment and we can iterate on them efficiently. |
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.
I just had a quick look. The majority of the contributions seem okay to me, but I feel I am missing something/misunderstanding the mechanism behind methods such as new_state_nowhere
. I think I might need to spend some more time into this (probably not today) before I can leave a fully informed review. In the meantime, if others have the time to review please go ahead, don't wait for my comments.
2 state creation interfaces (with/without location info) use optional, rank-agnostic, unlimited polymorphic inputs for easy setup of the error messages, e.g.: stdlib/example/linalg/example_state1.f90 Lines 7 to 8 in 41bf3a3
|
I tried to give a look and had a bit of trouble to fully grasp the mechanism. Also because I usually use very simple error handling. So my 1 cent at this stage would be that maybe a more complete example, (a kind of micro-tutorial) showing how it can be used together with an interface built locally within the example could help? |
I was thinking, for the example, say that you were to use this error catching mechanism to implement an interface for matrix inversion and you would like to decide wether to stop or just throw a warning if the determinant is zero? How would you structure the program? a small example showing this could be enlightening :) |
Great idea @jalvesz, I added one more example, I hope shows it better (later when we start merging linear algebra functions, there will be plenty more): stdlib/example/linalg/example_state2.f90 Lines 38 to 55 in 00db324
|
Thanks @perazz LGTM!! |
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.
LGTM. Thank you @perazz for this PR. I just have a few minor suggestions. Feel free to ignore them.
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
@perazz Should I generate a new release of stdlib (v0.5.0)? |
Yes @jvdp1, what do you think? I believe it's a good idea because that sets the starting point for the linear algebra procedures. Another option would be to wait until some of them are also implemented, but that could also come with |
I think it would set the starting point of the integration of BLAS/LAPACK into stdlib. This will help users to test it and provide some feedbacks. |
Yes, that sounds great @jvdp1! (If I am unresponsive here you can always reach me on Discourse). |
With this PR I aim to introduce a state/error handler for expert flow control of linear algebra procedures:
linalg_state
derived type with allpure
interfacescc: @fortran-lang/stdlib @jvdp1 @everythingfunctional @henilp105 @jalvesz @gnikit
This and #772 are prelminary to begin adding lapack-backed high-level algebra APIs, more context here.