Skip to content

Remove Vdotout functionality#54

Merged
jimmielin merged 2 commits intoKineticPreProcessor:devfrom
jimmielin:misc_cleanup
Jul 7, 2022
Merged

Remove Vdotout functionality#54
jimmielin merged 2 commits intoKineticPreProcessor:devfrom
jimmielin:misc_cleanup

Conversation

@jimmielin
Copy link
Copy Markdown
Member

@jimmielin jimmielin commented Jul 7, 2022

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.

@jimmielin jimmielin requested review from RolfSander and yantosca July 7, 2022 00:48
RolfSander
RolfSander reviewed Jul 7, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@RolfSander RolfSander left a comment •
edited
Loading

Choose a reason for hiding this comment

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

In CHANGELOG.md, the text "...it can be retrieved from Vloc" should be
changed to "...it can be retrieved from Vdot" because Vdot is the
dummy parameter.

@RolfSander
Copy link
Copy Markdown
Contributor

RolfSander commented Jul 7, 2022

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
yantosca approved these changes Jul 7, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@yantosca yantosca left a comment

Choose a reason for hiding this comment

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

This all looks good!

@yantosca
Copy link
Copy Markdown
Contributor

yantosca commented Jul 7, 2022

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
Copy link
Copy Markdown
Member Author

jimmielin commented Jul 7, 2022 •
edited
Loading

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:

5709ca3#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
Copy link
Copy Markdown
Contributor

RolfSander commented Jul 7, 2022

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...

@jimmielin jimmielin merged commit 7607565 into KineticPreProcessor:dev Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RolfSander RolfSander RolfSander left review comments

@yantosca yantosca yantosca approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jimmielin @RolfSander @yantosca