Use Y instead of C in Update_RCONST by srosanka · Pull Request #106 · KineticPreProcessor/KPP

srosanka · 2024-07-12T17:27:46Z

This pull request includes the necessary changes such that in Update_RCONST variable concentrations for rate constants that depend on species concentrations are used. To ensure backwards compatibility, YIN is an optional input. If YIN is not passed to Update_RCONST, concentrations from the global C array are used instead.

Closes #93

Update_RCONST now uses variable concentrations for rate constants that depend on species concentrations. To ensure backwards compatibility, YIN is an optional input. If YIN is not passed to Update_RCONST, concentrations from the global C array are used instead.

srosanka · 2024-07-12T17:40:36Z

@yantosca Should I add something to the documentation that Y should be used now instead of C when declaring reaction rates?

yantosca

@srosanka @RolfSander: I ran the C-I tests on the issue93 branch. I found a couple of issues:

We get a bunch of compile warnings in the C-language tests, that Y is an unused variable. It may be due to the fact that C doesn't allow for optional variables like F90 does.:

> C_rk_Rates.c: In function ‘Update_RCONST’:
> C_rk_Rates.c:166:8: warning: unused variable ‘Y’ [-Wunused-variable]
>   166 | double Y[NSPEC];                         /* Concentrations of species (local) */
>       |        ^
430a435,438
> C_rosadj_Rates.c: In function ‘Update_RCONST’:
> C_rosadj_Rates.c:166:8: warning: unused variable ‘Y’ [-Wunused-variable]
>   166 | double Y[NSPEC];                         /* Concentrations of species (local) */
>       |        ^
527a536,539
> C_sd_Rates.c: In function ‘Update_RCONST’:
> C_sd_Rates.c:166:8: warning: unused variable ‘Y’ [-Wunused-variable]
>   166 | double Y[NSPEC];                         /* Concentrations of species (local) */
>       |        ^
888a901,904
> C_sdadj_Rates.c: In function ‘Update_RCONST’:
> C_sdadj_Rates.c:166:8: warning: unused variable ‘Y’ [-Wunused-variable]
>   166 | double Y[NSPEC];                         /* Concentrations of species (local) */
>       |        ^
978a995,998
> C_small_strato_Rates.c: In function ‘Update_RCONST’:
> C_small_strato_Rates.c:166:8: warning: unused variable ‘Y’ [-Wunused-variable]
>   166 | double Y[NSPEC];                         /* Concentrations of species (local) */

The mcm example fails, but this is fixed in the h211b branch.

mcm_Rates.f90:431:19:

  431 |   USE constants_mcm
      |                   1
Error: Unexpected USE statement at (1)
make: *** [Makefile_mcm:238: mcm_Rates.o] Error 1
make: *** Waiting for unfinished jobs....

On the Azure Dev Pipelines we don't run the MCM example as that takes too much memory. So we can live with this until we merge in the h211b branch, I think.

Any thoughts about the warning when generating C-language code? We don't have a lot of C users for KPP but we probably should fix this for reliability's sake.

RolfSander · 2024-07-17T11:58:19Z

I was not able to reproduce the "unused variable" warning on my
computer. @yantosca: Maybe you are using a different C compiler version,
or maybe just a different warning level?

Anyway, maybe we can make the changes F90-specific and leave the code in
C and the other languages untouched. Something like this could work:

if ( useLang==F90_LANG )
    FunctionBegin( UPDATE_RCONST, YIN );
    Y = DefvElm( "Y", real, -NSPEC, "Concentrations of species (local)" );
    Declare(Y);
    NewLines(1);
else
    FunctionBegin( UPDATE_RCONST );

(note that I just typed this code here on github in pseudo-syntax)

yantosca · 2024-07-17T13:48:47Z

@srosanka: That would be great if you could add some documentation about using Y instead of C.

@RolfSander: I used gcc 10.2.0 and 11.4.0 in my tests. I can add the IF block around the code as you said.

RolfSander · 2024-07-17T14:00:13Z

I used gcc 10.2.0 and 11.4.0 in my tests.

I've used gcc 11.4.0, and I'm getting the warning now as well. I really
don't understand why I got no warning in my first try.

I can add the IF block around the code as you said.

Thanks!

yantosca · 2024-07-17T14:03:31Z

@RolfSander -- I can also update the C-I script to print the compiler version

RolfSander · 2024-07-17T14:08:36Z

Printing the compiler version is always a good idea when debugging (even though in this case, I don't think it can explain why I didn't see a warning initially).

src/gen.c - Wrap the code to declare UPDATE_RCONST with YIN in an if block that only executes when useLang==F90 - Place code to declare UPDATE_RCONST without any variables in the else block. Signed-off-by: Bob Yantosca <[email protected]>

.ci-pipelinoes/ci-common.defs.sh - Add a function "print_compiler_versions" to print the gcc and gfortran compiler versions that are used to run the tests. .ci-pipelines/ci-testing-script.sh - Call "print_compiler_versions" function before running tests. This will print the compiler versions at the top of stdout output. CHANGELOG.md - Updated accordingly Signed-off-by: Bob Yantosca <[email protected]>

yantosca · 2024-07-17T14:55:21Z

@RolfSander @srosanka: I pushed a fix that should only declare Update_Rconst with Y for F90.

Also the C-I tests now print at the top of stdout:

###########################################################################
         KPP CONTINUOUS INTEGRATION TESTS, USING THESE COMPILERS:

gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

GNU Fortran (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

###########################################################################

RolfSander · 2024-07-17T15:19:52Z

I pushed a fix that should only declare Update_Rconst with Y for F90.

Thanks, this looks good.

Also the C-I tests now print at the top of stdout

Thanks, this can be very helpful for future debugging!

srosanka · 2024-07-18T05:53:46Z

@yantosca and @RolfSander: thanks for spotting this issue and implementing the necessary changes to the code.

@yantosca: Great. I will then make some additions to the documentation.

srosanka · 2024-07-23T19:15:22Z

@yantosca I updated the documentation related to this issue. Please let me know if further adjustments are necessary.

RolfSander · 2024-07-24T09:29:06Z

Thanks, @srosanka, for writing the documentation!

However, there is one thing I don't understand. Why should the user
change C to Y in their *.eqn file? I thought that UPDATE_RCONST
now automatically gets the correct value for Y, and the user doesn't
have to change anything.

Maybe the new documentation should be moved from the explanation of the #EQUATIONS section to the
description of ICNTRL(15)?

yantosca · 2024-07-24T19:21:12Z

Thanks, @srosanka, for writing the documentation!

However, there is one thing I don't understand. Why should the user change C to Y in their *.eqn file? I thought that UPDATE_RCONST now automatically gets the correct value for Y, and the user doesn't have to change anything.

Maybe the new documentation should be moved from the explanation of the #EQUATIONS section to the description of ICNTRL(15)?

Thanks @srosanka and @RolfSander. I agree, I don't think we need to explicitly add Y to the rate constant. In the function template we have something like this:

!~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
SUBROUTINE FunTemplate( T, Y, Ydot, P_VAR, D_VAR )
!~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!  Template for the ODE function call.
!  Updates the rate coefficients (and possibly the fixed species) at each call
!~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
... 
   IF ( Do_Update_RCONST ) CALL Update_RCONST( Y )
...
END SUBROUTINE FunTemplate

so Y is already passed as an argument to the function that will be integrated.

RolfSander · 2024-07-25T07:10:59Z

I've adjusted the documentation. Is it ok now?

yantosca

Thanks @RolfSander! This is good to merge now.

srosanka · 2024-07-25T16:15:27Z

Thanks, @RolfSander and @yantosca for making the necessary adjustments to the documentation and for merging the code.

srosanka added the integrators Related to numerical integrators label Jul 12, 2024

srosanka added this to the 3.2.0 milestone Jul 12, 2024

srosanka requested a review from yantosca July 12, 2024 17:27

yantosca self-assigned this Jul 15, 2024

yantosca added the feature New feature or request label Jul 15, 2024

yantosca reviewed Jul 15, 2024

View reviewed changes

yantosca added 2 commits July 17, 2024 10:44

yantosca self-requested a review July 17, 2024 14:55

Update documentation related to #93

915b6f8

documentation adjusted

d459c4e

yantosca approved these changes Jul 25, 2024

View reviewed changes

yantosca merged commit f1d6ef1 into dev Jul 25, 2024

yantosca deleted the issue93 branch July 25, 2024 13:37

yantosca mentioned this pull request Sep 6, 2024

Are we using Update_RCONST() correctly? #93

Closed

Conversation

srosanka commented Jul 12, 2024

Uh oh!

srosanka commented Jul 12, 2024

Uh oh!

yantosca left a comment

Choose a reason for hiding this comment

Uh oh!

RolfSander commented Jul 17, 2024

Uh oh!

yantosca commented Jul 17, 2024

Uh oh!

RolfSander commented Jul 17, 2024

Uh oh!

yantosca commented Jul 17, 2024

Uh oh!

RolfSander commented Jul 17, 2024

Uh oh!

yantosca commented Jul 17, 2024

Uh oh!

RolfSander commented Jul 17, 2024

Uh oh!

srosanka commented Jul 18, 2024

Uh oh!

srosanka commented Jul 23, 2024

Uh oh!

RolfSander commented Jul 24, 2024

Uh oh!

yantosca commented Jul 24, 2024

Uh oh!

RolfSander commented Jul 25, 2024

Uh oh!

yantosca left a comment

Choose a reason for hiding this comment

Uh oh!

srosanka commented Jul 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants