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

[PULL REQUEST] Disable writing out of diaginfo.dat and tracerinfo.dat for WRF-GC. #165

Merged
merged 1 commit into from Dec 11, 2019

Conversation

jimmielin
Copy link
Contributor

Hi GCST,

Please find below a very simple fix that was causing massively slowdowns for WRF-GC. Details are in the original commit message below. This does not affect anything other than MODEL_WRF.

Thanks!
Haipeng

In WRF-GC, a bug in the preprocessor constant code in gc_environment_mod
caused a writeout of diaginfo and tracerinfo.dat files in a massively
parallel environment, because the code was only walled off for `ESMF_`
and not `MODEL_WRF`. This has now been fixed, as it was causing massive
slowdowns in WRF-GC initialization above hundreds of cores.

Signed-off-by: Haipeng Lin <hplin@seas.harvard.edu>

In WRF-GC, a bug in the preprocessor constant code in gc_environment_mod
caused a writeout of diaginfo and tracerinfo.dat files in a massively
parallel environment, because the code was only walled off for ESMF_
and not MODEL_WRF. This has now been fixed, as it was causing massive
slowdowns in WRF-GC initialization above hundreds of cores.

Signed-off-by: Haipeng Lin <hplin@seas.harvard.edu>
@jimmielin jimmielin changed the title Disable writing out of diaginfo.dat and tracerinfo.dat for WRF-GC. [PULL REQUEST] Disable writing out of diaginfo.dat and tracerinfo.dat for WRF-GC. Dec 10, 2019
@yantosca yantosca assigned yantosca and msulprizio and unassigned yantosca Dec 10, 2019
@sdeastham
Copy link
Contributor

I'd like to advocate for a MODEL_CLASSIC switch. Code like this is only relevant for GC-Classic, so it makes sense to specify that rather than de-specify every other implementation (even if this specific case is probably short-lived, being BPCH-related).

@jimmielin
Copy link
Contributor Author

I'd like to advocate for a MODEL_CLASSIC switch. Code like this is only relevant for GC-Classic, so it makes sense to specify that rather than de-specify every other implementation (even if this specific case is probably short-lived, being BPCH-related).

I remember we brought this up some time ago. It makes sense to have MODEL_CLASSIC or MODEL_GCHP, but then we are treating GCC/GCHP as specific "implementations" of the GEOS-Chem model. They kind of lose their "special status" as the main model, so whether or not add this kind of switch is more of a ideology problem.

I think in this particular case this code could be walled off without any pre-processor constant. Checking for am_I_Root is perfectly fine here and it will work magically with any implementation, because we all respect that flag.

@sdeastham
Copy link
Contributor

Should GC-Classic/GCHP have special status? Personally I do only see them as implementations - but I'm aware my perspective on this is pretty aggressive.

@lizziel
Copy link
Contributor

lizziel commented Dec 10, 2019

I think the diaginfo.dat and tracerinfo.dat code should be bracketed in a BPCH_DIAG cpp ifdef. These files are only useful if using binary diagnostics. That being said, we should also discuss cpp switches since there is room for improvement. Some of the existing ones can be removed or consolidated, and there are others we should probably have (I am a proponent of MODEL_CLASSIC). There will also be an increasing number of them as we interface with more ESMs. Ideally we would come up with a consistent philosophy about using them in advance and stick with it. I will start a separate discussion thread for this.

@msulprizio msulprizio merged commit 178931d into geoschem:dev/12.7.0 Dec 11, 2019
@msulprizio
Copy link
Contributor

This is now merged into dev/12.7.0.

@jimmielin @lizziel The call to DO_GAMAP which is blocked off here is already blocked off by an #ifdef BPCH_DIAG. Is BPCH_DIAG turned on for WRF-GC runs?

@jimmielin
Copy link
Contributor Author

jimmielin commented Dec 11, 2019

@msulprizio Hi Melissa, may I please ask where is the #ifdef BPCH_DIAG you mentioned that walls off DO_GAMAP? It should not be on for WRF-GC (we use netCDF diags) but somehow this was being ran.

Sorry, it was apparently added in a recent version. I was using 12.2.1 with WRF-GC for testing. Nevermind, thanks!

@msulprizio
Copy link
Contributor

No problem!

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

5 participants