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

Add array checking support for check interface #20

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

zoziha
Copy link

@zoziha zoziha commented Jun 18, 2022

Description

  • Add rank-1 arrays support for check interface;
  • Add new tests;
  • Add the pure keyword to pure functions.

Method: In order to avoid introducing double the number of check interfaces in testdrive for array checking, this PR uses class(*) to characterize the parameters. (+238 lines of code)

To close #19 , array checking is supported in this PR, but two problems are found, mainly in gfortran <= 10,

  • Problem 1: gfortran <= 10 seems to have limitations in the support of class(*) and associate(pointer => array);
  • Problem 2: The reshape of gfortran <= 10 may have adaptability problems with class(*).

After some adjustments, the above two problems are avoided, the content of this PR can theoretically pass gfortran >= 7 compilers. This means that if we want to support array checking and there is no better way, we need to remove support for gfortran 6 and add support for gfortran 12.

Help: But I can't solve the problem with cmake and meson config files, their CI always tells me that something went wrong, hope to get help, @awvwgk .(Solved)

@zoziha zoziha marked this pull request as draft June 18, 2022 15:40
@awvwgk
Copy link
Member

awvwgk commented Jun 19, 2022

The select type logic is not really that much shorter than having several duplicated procedures for dealing with one-dimensional arrays.

The test failures might be related to a routine which is guarded in the fpm case (WITH_XDP and WITH_QP) as those are disabled by default, while in meson / CMake they are enabled if compiler support is available.

item => array
do i = 1, size(array)
select type (item)
type is (integer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if you would agree to add fypp in this project, but I think it could be helpful for such a structure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it can be made work with all three build system (fpm, meson and CMake), I would be fine, however there is currently no way to support this intrinsically with fpm.

@codecov
Copy link

codecov bot commented Jun 20, 2022

Codecov Report

Attention: Patch coverage is 59.11330% with 83 lines in your changes missing coverage. Please review.

Project coverage is 66.41%. Comparing base (dc60eaf) to head (986fa4f).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
src/testdrive.F90 59.11% 0 Missing and 83 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #20      +/-   ##
==========================================
- Coverage   71.51%   66.41%   -5.11%     
==========================================
  Files           2        2              
  Lines         481      655     +174     
  Branches      296      436     +140     
==========================================
+ Hits          344      435      +91     
  Misses         23       23              
- Partials      114      197      +83     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zoziha
Copy link
Author

zoziha commented Jun 20, 2022

The select type logic is not really that much shorter than having several duplicated procedures for dealing with one-dimensional arrays.

At the beginning, I tried to implement multiple procedures for dealing with one-dimensional arrays, which probably added 600+ lines of code for test-drive.F90, I think it seems that select type may reduce the amount of code (+238 lines of code), if we worry about select type will degrade performance, or if we want to preserve gfortran 6 compatibility, we can change back to the multiple-procedure scheme.


By the way, now CI says: only gfortran 6 fails, codecov coverage is reduced.
gfortran 6 fails, probably not handling class(*), pointer and character(*) well:

 Starting character ... (86/89)

Program received signal SIGSEGV: Segmentation fault - invalid memory reference.

Backtrace for this error:
#0  0x7f5170f1cd57 in ???
#1  0x7f5170f1bfbd in ???
#2  0x7f51707ad0bf in ???
#3  0x7f51708f5764 in ???
#4  0x406bc1 in __testdrive_MOD_check_string
	at /src/test_drive.F90:1561
#5  0x40284c in __testdrive_MOD_check_double_array
	at /src/test_drive.F90:2099
#6  0x40e02f in test_char
	at /test/test_check_array.F90:1434
#7  0x40cb26 in run_unittest
	at /test/test_drive.F90:402
#8  0x40d20f in __testdrive_MOD_run_testsuite
	at /test/test_drive.F90:346
#9  0x4202d2 in tester
	at /test/main.f90:62
#10  0x4203e5 in main
	at /test/main.f90:18

@zoziha zoziha marked this pull request as ready for review June 20, 2022 01:29
@zoziha zoziha marked this pull request as draft June 20, 2022 12:51
@zoziha zoziha marked this pull request as ready for review June 20, 2022 13:16
@zoziha
Copy link
Author

zoziha commented Jun 20, 2022

@awvwgk, @jvdp1, maybe the latest commit will make us more satisfied:

  1. For a single array check, select type is still used, which is concise enough and introduces a small amount of code;
  2. For two array checks, multiple procedures are used, which satisfies the compatibility of gfortran 6 and increases the amount of code to a certain extent.

It would be nice to see that test-drive supports array checking. Arrays are a common data structure in Fortran. We can easily reshape any rank array into a rank-1 array for checking:

call check(error, reshape(array, [size(array)], expected, ...)

PS. In terms of efficiency, test-drive makes xdp and qp precision optional. Due to the addition of array checking, the check interface has become heavier. In our daily calculations, real numbers seem to be used more than integers. Do we need to set optionals for integers, like integer(i1,i2) as optionals? Of course, if we let it go, it's fine.

awvwgk
awvwgk previously requested changes Jun 20, 2022
Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a check for the shape of the arrays to avoid out-of-bounds access in case the expected array is larger than the actual or elements remain unchecked in case the actual array is larger.

@zoziha
Copy link
Author

zoziha commented Jun 21, 2022

Thank you for review. This commit has added a check on the size of the array. Since my native language is not English, if there are some mistakes in the text, please point them out.

summary:

  • The first commit, adding array support, a total of +238 lines of code;
  • The second commit, partially use the multi-procedures scheme, a total of +633 lines of code;
  • The third commit, adding a check on the array size, a total of +701 lines of code.

(Hope it doesn't add too much code burden.)

@HugoMVale
Copy link

Hello. Just want to state that I find this new feature quite useful, actually a must. I am using it, with success, in my own code since yesterday (I was about to write something similar myself at least for dp arrays).
Is there any special reason why the merge is pending? Thanks.

@awvwgk awvwgk self-requested a review August 29, 2022 19:38
@awvwgk
Copy link
Member

awvwgk commented Aug 29, 2022

Is there any special reason why the merge is pending?

There is nothing fundamentally blocking this patch, if you want to move this forward, feel free to help out with the code review.

@HugoMVale
Copy link

@awvwgk, I only have a superficial understanding of the PR/review/approval process. But there is a first time for everything, and I am happy to learn. Could I ask you to point me in the right direction? How exactly can I help? How do I start the process? Thanks... and sorry for the beginner questions.

@awvwgk awvwgk dismissed their stale review August 29, 2022 20:19

Requested changes addressed

@zoziha
Copy link
Author

zoziha commented Aug 30, 2022

My main concern is that although the current solution caters to the coding paradigm of test-dive, such a heavy API may be unsustainable in the future and destroy the simplicity of the original test-drive, but I can't think of a better way .

Not supporting arrays is fine if we stick to test-drive brevity.

@HugoMVale
Copy link

@zoziha, for sure, this new feature adds a non-negligible number of new functions and code lines, but IMO this burden is outweighed by the benefits of the feature itself. Moreover, IMO, the maintainability issue is not really due to this new addition, but something that was already there. For instance, the functions check_float_sp, check_float_dp, check_float_xdp, check_float_qp, are already "almost-perfect-copies" of each other except for 2-3 characters. Any correction/improvement will involve a lot of copy-paste... but this is not a problem of the code - it's rather Fortran's way.
Regarding the code itself, some questions/suggestions:

  • call check(error, size(actual), size(expected), message, more) -> call check_int_i4(error, size(actual), size(expected), message, more) ?;
  • Similarly, inside each new check_type_r1 subroutine, could we not replace call check(error, actual(i), expected(i), message, more) by the specific type subroutine check_type, in order to avoid passing each time through the interface.

Note sure if this change is of any computational benefit, but it does seem more explicit.

@zoziha zoziha marked this pull request as draft August 30, 2022 15:16
@zoziha zoziha marked this pull request as ready for review August 30, 2022 15:24
@zoziha
Copy link
Author

zoziha commented Aug 30, 2022

Any correction/improvement will involve a lot of copy-paste... but this is not a problem of the code - it's rather Fortran's way.

@HugoMVale, yes, I agree with you. In the existing Fortran grammar, if some generic features are to be supported, certain trade-offs are often required.
Thanks for the suggestion, it is possible to use module procedures, which are more explicit, and the patch has been updated now.

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.

Support array checking for the check subroutine
4 participants