KineticPreProcessor / KPP

The KPP kinetic preprocessor is a software tool that assists the computer simulation of chemical kinetic systems
GNU General Public License v3.0
21 stars 11 forks source link

[PULL REQUEST] Replace all F77-style EQUIVALENCE statements with pointer assignments -- Closes #27 #33

Closed yantosca closed 2 years ago

yantosca commented 2 years ago

This PR corresponds to feature request https://github.com/KineticPreProcessor/KPP/issues/27. For Fortran-90 only, we have removed the EQUIVALENCE statement in the _Global module and have replaced that with pointer assignments.

  1. Global variable C is now declared with the TARGET attrbitue
  2. Global variables VAR and FIX are now declared with the POINTER attribute.
  3. VAR and FIX are now assigned and nullified in each integrator routine int/*.f90
  4. In drv/driver_tlm.f90 and drv/driver_adj.f90, replace VAR with C(1:NVAR).
  5. Modified the KPP C routines to pass an additional argument to DefineVariable that is used to determine if a variable is a F90 pointer or target.
  6. Updated comments and cleaned up code, especially in int/*.f90.
yantosca commented 2 years ago

@RolfSander, here is a new PR to dev. Thanks.

yantosca commented 2 years ago

@jimmielin, @msl3v, let me know if any of this will conflict with your autoreduce code.

yantosca commented 2 years ago

I looked at the code and I also tested the new pointers with my MECCA model, and this all looks fine.

I do, however, have a couple of comments on the merge procedure. The removeEquivalence branch was created before add-geos-chem-updates was merged into dev. I therefore expect several merge conflicts. A possible solution could be to merge the current dev into removeEquivalence first. Then we should have all the latest changes inside removeEquivalence. This is how I usually try to work with git. If anyone has an alternative idea, I'm open for everything.

I can merge dev into removeEquivalence and then resolve conflicts. Stay tuned.

While checking the code, I stumbled across some other, pretty minor points. It's probably not necessary to fix them now. I'm just writing them down here so I won't forget:

  • In kpp_global.f90, we have this comment: The following quantities do not vary with location. This is correct for GEOSCHEM and also for our 3D-model. However, I'd like to avoid any model-specific statements. For example, someone may perform 1000 Monte-Carlo simulations with different rate constants. For them, there would be no "location".

Good point. I was trying to explain the reason for the threadprivates. I could modify it to make it more generic like, "for models using KPP on a geospatial grid, these variables will have to be held threadprivate..."

  • Why is Zero multiplied by CFACTOR in kpp_initialize.f90? Does it help to cast the numeric value into the right precision? I cannot see any other possible explanation.

I think that is what is in the Initialize template code. I can take it out.

  • I noticed that very long equations are not presented correctly in kpp_monitor.f90. Instead of a complete listing of all products, there should be "... etc." at the end. I think I have to fix this in gen.c.

OK no worries.

RolfSander commented 2 years ago

I can merge dev into removeEquivalence and then resolve conflicts. Stay tuned.

Thanks!

I could modify it to make it more generic like, "for models using KPP on a geospatial grid, these variables will have to be held threadprivate..."

Hmm, geospatial is still specific to the geosciences. What is the real reason from a computer science point of view? Is it "using KPP inside a parallel environment"?

I think that is what is in the Initialize template code. I can take it out.

Yes, and it has always been in gen.c:

F90_Inline(" C = (0.)*CFACTOR" );

I was just wondering what it meant...

yantosca commented 2 years ago

dev is now merged into feature/removeEquivalenceStatements

Hmm, geospatial is still specific to the geosciences. What is the real reason from a computer science point of view? Is it "using KPP inside a parallel environment"?

Yes, that's right.... I could modify it to say that if you are using KPP within an OpenMP parallel environment, then these variables need to be held threadprivate.

RolfSander commented 2 years ago

if you are using KPP within an OpenMP parallel environment

This sounds good and very generic.

RolfSander commented 2 years ago

dev is now merged into feature/removeEquivalenceStatements

Thanks! AFAICS, the only thing still missing is the description of #UPPERCASEF90 in kpp_UserManual.tex.

yantosca commented 2 years ago

The comments now read:

! Declaration of global variables

! ~~~ If you are using KPP within an OpenMP parallel environment,
! ~~~ then these variables must be declared THREADPRIVATE.  This means
! ~~~ that the compiler will make a private copy of these variables
! ~~~ (in stack memory) for each execution thread.  At the end of
! ~~~ the OpenMP parallel loop, these variables will be finalized,
! ~~~ and their memory deallocated.
! ~~~
! ~~~ NOTE: Because the OpenMP commands all begin with a comment
! ~~~ character, they will be ignored unless the code is compiled
! ~~~ with OpenMP parallelization turned on.

and

! ~~~ If you are using KPP within an OpenMP parallel environment,
! ~~~ these variables DO NOT need to be declared THREADPRIVATE.

Also I have modified gen.c so we print out either C = 0.0_dp or C = 0.0 without multiplying by CFACTOR (for F90 only).

Thanks for the feedback, I appreciate it!

yantosca commented 2 years ago

UPPERCASEF90 in kpp_UserManual.tex.

I'll add that.

yantosca commented 2 years ago

User manual updated.