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

Remove Vdotout functionality #54

Closed jimmielin closed 2 years ago

jimmielin commented 2 years ago

As discussed in #50, Vdotout can be obsoleted and replaced with Vdot in Fun().

Hi @yantosca, it looks like Vdotout can probably be removed, if there are no more bindings to it in GEOS other than fullchem_mod.F90. I looked at the dev branch and the only place we use Vdotout is:

          CALL Fun( C(1:NVAR), C(NVAR+1:NSPEC), RCONST,                          &
                    Vloc,      Aout=Aout,       Vdotout=Vdotout )
          NOxTau = Vdotout(ind_NO) + Vdotout(ind_NO2) + Vdotout(ind_NO3)         &
                 + 2.*Vdotout(ind_N2O5) + Vdotout(ind_ClNO2) + Vdotout(ind_HNO2) &
                 + Vdotout(ind_HNO4)

Some other calls get Vdotout but they're not used anywhere (and it's a local variable so it doesn't propagate to other places). It can probably be replaced with Vloc which has the same value as Vdotout, and we can get rid of an OPTIONAL in Fun():

We don't have to remove this right away in KPP 3.0.0.rc.1 - I'll open a PR for 3.0.0 to get rid of Vdotout and we can do the changes in GEOS-Chem downstream first then merge into KPP later.

This update requires accompanying updates on the GEOS-Chem side, namely replacing Vdotout by Vloc here.

We probably don't have to merge this into 3.0.0 right away if there is no time / there is a code freeze for GEOS-Chem 14. But it should be a relatively minor change to make.

RolfSander commented 2 years ago

One more thing: When we're done with Vdotout, we should check if this affects the Matlab code as well (one of the matlab fixes that I recently applied was related to Vdotout).

yantosca commented 2 years ago

Also, GEOS-Chem 14.0.0 will use KPP 2.5.0. So we could bring this in now, and I can fix fullchem_mod.F90 in GEOS-Chem for a future version.

jimmielin commented 2 years ago

One more thing: When we're done with Vdotout, we should check if this affects the Matlab code as well (one of the matlab fixes that I recently applied was related to Vdotout).

Hi @RolfSander, I just checked the Matlab fix:

https://github.com/KineticPreProcessor/KPP/commit/5709ca39462c41d12143278901f93ff0c155e5c7#diff-75f594008bcd3db69d19f27727f5f2ea2d033a589b10527b248f96ef16b01272L841

It looks like support for Vdotout was added in MATLAB in this commit. I simply removed this feature for MATLAB, replacing

  if( z_useAggregate )
    MATLAB_Inline("\n   Vdotout = Vdot(:);\n");
  else
    MATLAB_Inline("\n   P_VAR = P_VAR(:);\n   D_VAR = D_VAR(:);\n");

with

  if( !z_useAggregate )
    MATLAB_Inline("\n   P_VAR = P_VAR(:);\n   D_VAR = D_VAR(:);\n");

Does this look good? We should probably have some CI-tests for Matlab in the future, maybe using GNU Octave. Maybe it will run with Octave, maybe not, I can give it a try.

I'll update the wrong note I added in the documentation shortly.

RolfSander commented 2 years ago

I'm not sure if Vdotout was really needed in Matlab. I think I'll just ask our Matlab colleagues.

It would be nice if the Matlab code runs in Octave...