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

Fix an error in COSP 1.4 MODIS simulator #2097

Merged
merged 2 commits into from
Apr 3, 2018

Conversation

wlin7
Copy link
Contributor

@wlin7 wlin7 commented Feb 21, 2018

This update fixes an error in the calculation of NIR reflectance that affects cloud fields derived from MODIS simulator. As a tentative arrangement, the fixes are only effective when the CPP directive
-DAPPLY_POST_DECK_BUGFIXES is applied to CAM_CONFIG_OPTS, which can be specified in compsets of interest in components/cam/cie_config/config_components.xml.

The impact is generally small.

Modified: components/cam/src/physics/cosp/MODIS_simulator/modis_simulator.F90

[non-BFB] for COSP-MODIS simulator diagnostics output when the CPP directive is defined
[BFB] all else

 This update fixes an error in the calculation of NIR reflectance that affects cloud fields derived from MODIS simulator.
 The impact is generally small.

 modified:   components/cam/src/physics/cosp/MODIS_simulator/modis_simulator.F90

 BFB
@wlin7 wlin7 added Atmosphere bug fix PR BFB PR leaves answers BFB labels Feb 21, 2018
singhbalwinder added a commit that referenced this pull request Feb 21, 2018
Fix an error in COSP 1.4 MODIS simulator

This update fixes an error in the calculation of NIR reflectance that
affects cloud fields derived from MODIS simulator.The impact is
generally small.

modified: components/cam/src/physics/cosp/MODIS_simulator/modis_simulator.F90

[BFB]

* wlin/atm/cosp-modis-1.4.2fix:
  Fix an error in COSP 1.4 MODIS simulator
@singhbalwinder
Copy link
Contributor

merged to next

@singhbalwinder
Copy link
Contributor

@wlin7 : Do you think it will be NBFB for COSP tests? I see a failed test on CDASH and was wondering if this PR broke that test.

@singhbalwinder
Copy link
Contributor

singhbalwinder commented Feb 27, 2018

I talked with @wlin7 offline and we found out that the following diagnostic variables are causing the COSP test to break:

CLHMODIS CLIMODIS CLMMODIS CLMODIS CLTMODIS IWPMODIS LWPMODIS PCTMODIS REFFCLIMODIS REFFCLWMODIS TAUILOGMODIS TAUIMODIS TAUTLOGMODIS TAUTMODIS

@PeterCaldwell , @golaz and @rljacob : Is it okay to merge this NBFB PR?

@wlin7 wlin7 added non-BFB PR makes roundoff changes to answers. and removed BFB PR leaves answers BFB labels Feb 27, 2018
@rljacob
Copy link
Member

rljacob commented Feb 28, 2018

This may have to wait until post v1. The CMIP6 runs are all being done with COSP output (and without this fix) and v1 should give the same values.

@rljacob
Copy link
Member

rljacob commented Feb 28, 2018

@wlin7 please open an issue that describes the problem this PR fixes.

@wlin7
Copy link
Contributor Author

wlin7 commented Feb 28, 2018

@rljacob , the issue #2113 has been created to describe this.

Since the bulk of the CMIP6 simulations are yet to be completed, it is very meaningful, as long as still possible, to include this fix for V1 simulations.

Only the diagnostic COSP MODIS output are affected and a pair of test simulations show the impact is limited.

@rljacob
Copy link
Member

rljacob commented Feb 28, 2018

Its up to @golaz

@singhbalwinder
Copy link
Contributor

Thanks @rljacob and @wlin7 . This PR is already merged to next and breaking a test. Should I revert it back until we hear back from @golaz ?

@golaz
Copy link
Contributor

golaz commented Feb 28, 2018

Since the low-res DECK simulations are already 1/3 done, I don't think we can change this mid-course. The report states that "the impact is generally small", so does it mean we can still do useful science with the COSP MODIS simulator output?

@zyuying
Copy link
Contributor

zyuying commented Mar 1, 2018

@golaz This is the bug in the released COSP. It was recently found by the COSP MODIS simulator developer. He just notified COSP users of the bug in the simulator code and sent the patch on Feb 1st. The error affects all MODIS simulator variables only when clouds with both liquid and ice visible from space. It was suggested by them to either update the COSP with the patch or submit to CMIP6/CFMIP3 with affected diagnostics omitted without the patch. Based on their suggestion, I would suggest we deal with this until post v1.

@wlin7
Copy link
Contributor Author

wlin7 commented Mar 1, 2018

Hi @golaz , @zyuying , The "generally small impact" was based on the contrast of two 15- month simulations, and it could still be small over longer term simulation. But given the developer's suggestion, they would consider all data unusable without patch.

COSP MODIS is useful for a number of cloud diagnostics, like the variables @singhbalwinder listed. It can still be very meaningful (and likely more than sufficient) for any practical interest of analyzing MODIS simulator output if 2/3 of each DESK simulations have good MODIS output. There will definitely be inconvenience switching the code base in the middle of the run, though.

@rljacob rljacob added the postV1 Do not merge until after v1 tag label Mar 6, 2018
singhbalwinder added a commit that referenced this pull request Mar 6, 2018
…)"

This reverts commit 67cdd0b, reversing
changes made to 3b1b293.

This PR is now a post V1 candidate. It was breaking a COSP test and
non-BFB changes are not allowed at this point.
@singhbalwinder
Copy link
Contributor

Reverted from next. This PR is now a Post V1 candidate.

@wlin7
Copy link
Contributor Author

wlin7 commented Mar 25, 2018

Hi @singhbalwinder , @rljacob ,

We now want to apply the known bug fixes (such as this one, and a radiation imbalance fix in clm, which involves multiple files and is in branch bishtgautam/lnd/radiation-fix).

The idea is to apply these fixes in high-res coupled runs using 1950 compsets, because they are expensive to run ( and to avoid re-run). A flag must be implemented to switch the modified codes on and off.

If using a single namelist variable, e.g., apply_post_deck_bugfixes, because it needs to be used by multiple model components, I initially thought of adding it to infodata in cime, just like the gustiness parameter 'gust_fac'. But whether it works may depend on PE settings.

The simplest I can think of is to use CPP macro like -DAPPLY_POST_DECK_BUGFIXES. It can be added to components/cam[clm]/cime_config/config_component.xml in CAM_CONFIG_OPTS and CLM_CONFIG_OPTS for atm and lnd, respectively, for certain compsets (e.g., 1950_CAM5%CMIP6-HR_CLM45).

Other than the above, the flag can be implemented as namelist variable for atm and lnd components separately, and pass it down the code flow to the destination routines.

Your opinions on these are much appreciated.

Fix an error in COSP 1.4 MODIS simulator

This update fixes an error in the calculation of NIR reflectance that affects cloud fields derived from MODIS simulator.
As a tentative arrangement, the fixes are only effective when the CPP directive -DAPPLY_POST_DECK_BUGFIXES is applied to
CAM_CONFIG_OPTS, which can be specified in compsets of interest in components/cam/cie_config/config_components.xml.

The impact is generally small.

Modified:   components/cam/src/physics/cosp/MODIS_simulator/modis_simulator.F90

BFB for all that are not output from MODIS simulator when the CPP directive is defined.
@wlin7
Copy link
Contributor Author

wlin7 commented Mar 28, 2018

@singhbalwinder , just like PR #2202 , the CPP directive APPLY_POST_DECK_BUGFIXES have been added to this PR, and putting back the codes before the fix. The two pieces of codes are controlled by the CPP directive which will be set separately for selected compsets.

@rljacob rljacob removed the postV1 Do not merge until after v1 tag label Mar 30, 2018
@wlin7
Copy link
Contributor Author

wlin7 commented Apr 2, 2018

@rljacob , @bishtgautam , I ran into merge conflict for this PR, just like when merging #2124. This PR was previously merged to next but then reverted for post V1. The codes are now further updated with CPP directive.

Revert the revert than merge does not work this time. Can you please help with this? Thanks.

@rljacob
Copy link
Member

rljacob commented Apr 2, 2018

Another approach would be to rebase this branch. Then you can just merge without a revert-revert.

singhbalwinder added a commit that referenced this pull request Apr 2, 2018
Fix an error in COSP 1.4 MODIS simulator

This update fixes an error in the calculation of NIR reflectance that
affects cloud fields derived from MODIS simulator. As a tentative
arrangement, the fixes are only effective when the CPP directive
-DAPPLY_POST_DECK_BUGFIXES is applied to CAM_CONFIG_OPTS, which
can be specified in compsets of interest in
components/cam/cie_config/config_components.xml.

The impact is generally small.

Modified: components/cam/src/physics/cosp/MODIS_simulator/
modis_simulator.F90

[non-BFB] for COSP-MODIS simulator diagnostics output
when the CPP directive is defined
[BFB] all else

* wlin/atm/cosp-modis-1.4.2fix:
  COSP-MODIS bugfix with APPLY_POST_DECK_BUGFIXES directive

Conflicts:
	components/cam/src/physics/cosp/MODIS_simulator/
modis_simulator.F90
@singhbalwinder
Copy link
Contributor

I have resolved the conflict and merged this PR to next.

@singhbalwinder singhbalwinder merged commit bc6f1f1 into master Apr 3, 2018
singhbalwinder added a commit that referenced this pull request Apr 3, 2018
Fix an error in COSP 1.4 MODIS simulator

This update fixes an error in the calculation of NIR reflectance that
affects cloud fields derived from MODIS simulator. As a tentative
arrangement, the fixes are only effective when the CPP directive
-DAPPLY_POST_DECK_BUGFIXES is applied to CAM_CONFIG_OPTS, which
can be specified in compsets of interest in
components/cam/cie_config/config_components.xml.

The impact is generally small.

Modified: components/cam/src/physics/cosp/MODIS_simulator/
modis_simulator.F90

[non-BFB] for COSP-MODIS simulator diagnostics output
when the CPP directive is defined
[BFB] all else

* wlin/atm/cosp-modis-1.4.2fix:
  COSP-MODIS bugfix with APPLY_POST_DECK_BUGFIXES directive
  Fix an error in COSP 1.4 MODIS simulator
@wadeburgess wadeburgess deleted the wlin/atm/cosp-modis-1.4.2fix branch April 5, 2018 19:56
wlin7 added a commit that referenced this pull request Apr 28, 2018
Post deck bug fixes in 3 PRs -- #2097 for COSP MODIS, #2202 for RRTMG limiters, and # 2204 for clm radiation -- were made effective
for 1950 compsets before the v1 release using CPP directive -DAPPLY_POST_DECK_BUGFIXES.
This PR is to make those bug fixes default for all configurations. The CPP directive and the
companion switching blocks are removed. The specifications of CPP directive for 1950 compsets in CAM or CLM CONFIG options are also removed.
The RRTMG files involved that were previously renamed from .f90 to .F90, in order to let Fortran compilers (such as Intel on cori) invoke
preprocessor, are renamed back to .f90.

	modified:   components/cam/cime_config/config_component.xml
	modified:   components/cam/src/physics/cosp/MODIS_simulator/modis_simulator.F90
	renamed:    components/cam/src/physics/rrtmg/ext/rrtmg_lw/rrtmg_lw_setcoef.F90 -> components/cam/src/physics/rrtmg/ext/rrtmg_lw/rrtmg_lw_setcoef.f90
	renamed:    components/cam/src/physics/rrtmg/ext/rrtmg_mcica/rrtmg_sw_spcvmc.F90 -> components/cam/src/physics/rrtmg/ext/rrtmg_mcica/rrtmg_sw_spcvmc.f90
	renamed:    components/cam/src/physics/rrtmg/ext/rrtmg_sw/rrtmg_sw_reftra.F90 -> components/cam/src/physics/rrtmg/ext/rrtmg_sw/rrtmg_sw_reftra.f90
	renamed:    components/cam/src/physics/rrtmg/ext/rrtmg_sw/rrtmg_sw_setcoef.F90 -> components/cam/src/physics/rrtmg/ext/rrtmg_sw/rrtmg_sw_setcoef.f90
	modified:   components/clm/cime_config/config_component.xml
	modified:   components/clm/src/biogeophys/BalanceCheckMod.F90
	modified:   components/clm/src/biogeophys/CanopyHydrologyMod.F90
	modified:   components/clm/src/biogeophys/EnergyFluxType.F90
	modified:   components/clm/src/biogeophys/SnowHydrologyMod.F90
	modified:   components/clm/src/biogeophys/SoilFluxesMod.F90
	modified:   components/clm/src/biogeophys/SoilTemperatureMod.F90
	modified:   components/clm/src/biogeophys/SurfaceAlbedoMod.F90
	modified:   components/clm/src/biogeophys/SurfaceRadiationMod.F90

[non-BFB] except for 1950 compsets
singhbalwinder added a commit that referenced this pull request May 22, 2018
…2313)

Makes default the post deck bug fixes enabled via CPP for 1950 compsets

Post deck bug fixes in 3 PRs -- #2097 for COSP MODIS, #2202 for RRTMG
limiters, and #2204 for clm radiation -- were made effective for 1950
compsets before the v1 release using CPP directive
-DAPPLY_POST_DECK_BUGFIXES. This PR is to make those bug fixes default
for all configurations.

The CPP directive and the companion switching blocks are removed. The
specifications of CPP directive for 1950 compsets through CAM or CLM
CONFIG_OPTS (PR #2222) are also removed.

The RRTMG files involved that were previously renamed from .f90 to .F90,
 in order to let Fortran compilers (such as Intel on cori) invoke
preprocessor, are renamed back to .f90.

[non-BFB] except for 1950 compsets

* wlin/atmlnd/postdeck-fixes-rm-cpp-1950:
  Makes default the post deck bug fixes enabled via CPP for 1950 compsets
singhbalwinder added a commit that referenced this pull request May 23, 2018
Makes default the post deck bug fixes enabled via CPP for 1950 compsets

Post deck bug fixes in 3 PRs -- #2097 for COSP MODIS, #2202 for RRTMG
limiters, and #2204 for clm radiation -- were made effective for 1950
compsets before the v1 release using CPP directive
-DAPPLY_POST_DECK_BUGFIXES. This PR is to make those bug fixes default
for all configurations.

The CPP directive and the companion switching blocks are removed. The
specifications of CPP directive for 1950 compsets through CAM or CLM
CONFIG_OPTS (PR #2222) are also removed.

The RRTMG files involved that were previously renamed from .f90 to .F90,
 in order to let Fortran compilers (such as Intel on cori) invoke
preprocessor, are renamed back to .f90.

[non-BFB] except for 1950 compsets

* wlin/atmlnd/postdeck-fixes-rm-cpp-1950:
  Makes default the post deck bug fixes enabled via CPP for 1950 compsets
brhillman pushed a commit that referenced this pull request Jan 25, 2023
Automatically Merged using E3SM Pull Request AutoTester
PR Title: SHOC assumed PDF NaN Output dump
PR Author: bogensch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Atmosphere bug fix PR non-BFB PR makes roundoff changes to answers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants