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

Fixes for extended and quad precision checking. Add CI cheks with fpm #821

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

jalvesz
Copy link
Contributor

@jalvesz jalvesz commented May 18, 2024

After looking at #819 I realized that there were several issues:

  1. in the _specialfunctions_gamma files, there were this kind of fypp macros:
#:set WITH_QP = False
#:set WITH_XDP = False
#:include "common.fypp"

Which implied that the different kind definitions were being redefined. In parallel, this means that some files were correctly preprocessed and others not, depending on when the problematic files were being preprocessed.
2. many tests are not using fypp "properly" so I refactored those that I catched after the modifications
3. Seems like there is no CI checking xdp, so I propose to use the fpm CI to check xdp and qp with gfortran.
4. I don't know if test-drive is not built with xdp and qp, but the call check( ... ) using the tolerance argument thr had to be modified to avoid passing xdp or qp arguments.

I think this is ready for consideration @perazz @jvdp1 @nsh-envs

@jalvesz jalvesz marked this pull request as draft May 18, 2024 07:21
@jalvesz jalvesz marked this pull request as ready for review May 18, 2024 08:16
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.

Thank you @jalvesz .
I only have a minor commnet, regarding the robustness of the definition of the variable RC_KINDS_TYPES.

src/stdlib_specialfunctions_gamma.fypp Outdated Show resolved Hide resolved
src/stdlib_specialfunctions_gamma.fypp Outdated Show resolved Hide resolved
@perazz
Copy link
Contributor

perazz commented May 18, 2024

I agree with @jvdp1: great work @jalvesz, thank you for finding this out! I don't want to have you rewrite your PR but here is some thought.
Elsewhere because LAPACK also does not support xdp, I've used this approach:

#:for rk,rt in RC_KINDS_TYPES
#:if rk!="xdp"

So here, it could be something like this?

#:if rk not in ["xdp","qp"]

Also: the special functions just want to be for the "core" types (32- and 64-bit).
So I'm not suggesting to do it in this PR, but would there be a way we could unify this once and forall? For example, by having some CORE_REAL_KINDS defined in common.fypp as opposed to all REAL_KINDS?

#! Real kinds to be considered during templating
#:set REAL_KINDS = ["sp", "dp"]
#:if WITH_XDP
#:set REAL_KINDS = REAL_KINDS + ["xdp"]
#:endif
#:if WITH_QP
#:set REAL_KINDS = REAL_KINDS + ["qp"]
#:endif

@awvwgk
Copy link
Member

awvwgk commented May 19, 2024

For test-drive to accept extended double and quadruple precision the C preprocessor macros WITH_XDP=1 and WITH_QP=1 need to be set when compiling test-drive.

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.

Thank ypu @jalvesz .

src/stdlib_specialfunctions_gamma.fypp Outdated Show resolved Hide resolved
src/stdlib_specialfunctions_gamma.fypp Outdated Show resolved Hide resolved
#:include "common.fypp"
#:set CI_KINDS_TYPES = INT_KINDS_TYPES + CMPLX_KINDS_TYPES
#:set R_KINDS_TYPES = [KT for KT in REAL_KINDS_TYPES if KT[0] in ["sp","dp"]]
Copy link
Member

Choose a reason for hiding this comment

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

LGTM.
Did you find a way to compile test-drive with/out QP and XDP if supported or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jvdp1 I didn't have time last week to look at that. I guess this should be done by propagating the fypp macros backwards in the dependency hierarchy but right now I do not see clearly how to do that without touching the build of test-drive itself or transferring the python preprocessing script to fpm.

Copy link
Member

@jvdp1 jvdp1 Jun 1, 2024

Choose a reason for hiding this comment

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

I guess it should be also in the CMake files. I will have a look.
Edit: the flags are already used for test-drive with CMake

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we then have one of the cmake builds test xdp and qp instead of the fpm build?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, good idea!
But the issue might still be there if someone try the tests with fpm, right? Or do I miss something?

Copy link
Contributor Author

@jalvesz jalvesz Jun 1, 2024

Choose a reason for hiding this comment

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

Yes, it would still be there. The ideas on top of my head are: add the python preprocessing to test-drive as well or add it to fpm (it might be the most robust solution but I'm guessing not the easiest)

Edit: I looked at test-drive sources, so since it uses only C preprocessing and not fypp this should be actually easier. I'll check how to propagate the flags from the stdlib fpm build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing the corresponding flags in the build fpm test --profile release --flag '-DWITH_XDP -DWITH_QP' are indeed enough to backpropagate the option.

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.

Thank you @jalvesz . Pending a minor comment, I think this PR can be merged.

test/quadrature/test_simps.fypp Show resolved Hide resolved
@jvdp1
Copy link
Member

jvdp1 commented Jun 11, 2024

@jalvesz It sounds it is ready to be merged. We will wait a few more days, and then merge it. Thank you.

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.

None yet

4 participants