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

Include the option of McICA for the shortwave component of RRTMG #83

Merged
merged 17 commits into from
Sep 17, 2018

Conversation

SallyDa
Copy link
Contributor

@SallyDa SallyDa commented Sep 3, 2018

I got stuck pretty much straight away and asked @skosukhin for help. He kindly agreed and decided to start from the original rrtmg files and to change those files as little as possible, to make it easier to update to a newer version of rrtmg in the future. Both the nomcica and mcica versions work and produce results very similar to those from @JoyMonteiro 's implementation of the nomcica rrtmg, except in the uppermost model level. I don't know why.

On the python side, I added a dictionary with the extra properties required for mcica, but I expect you might want to move this and add the * dimension. Let me know if you have any comments or suggestions and I can try to include them, or feel free to edit it yourselves!

Oh and I have no idea what the input parameter iplon in the mcica version of rrtmg is, but actually I don't think it does anything anyway...

@mcgibbon
Copy link
Member

mcgibbon commented Sep 3, 2018

We need to get the code passing tests. This means a couple of things:

  • go into the code directory and run flake8. If you don't have it installed, pip install flake8 first. This will tell you any code style violations you need to fix.
  • then, go into the tests directory and run py.test. Again, if you don't have it installed, pip install pytest first. This will run all our existing tests, make sure none break and if they do try to find out why. If tests break that are unrelated to RRTMG you may want to post here so we can help figure it out.
  • Once you're sure no existing tests now fail, you can make new tests for this change. You should go into test_components.py and add a test class like
class TestRRTMGShortwaveMCICA(ComponentBaseColumn, ComponentBase3D):
    def get_component_instance(self):
        return RRTMGShortwave(mcica=True)
  • Re-run the tests either with py.test or python test_components.py. The first time you run these tests some will fail as they generate cached output to check against future times the tests are run. Run them a second time, and fix the code to pass any tests that fail this second time. This is only an absolutely bare test that makes sure the code runs without crashing, and that each time you run it it behaves the same.
  • Also make an example script that uses this option in a scientific context. Make sure to run it and see that it produces reasonable output (plots, etc.). This is a better test of whether the code is actually doing what it's intended to do, and not just running without errors.


num_reduced_g_intervals = self.num_reduced_g_intervals
mid_levels = state['air_pressure'].shape[0]
mcica_properties = {
Copy link
Member

Choose a reason for hiding this comment

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

Should these be set to zero, or taken as inputs and put into input_properties? Will this even work with a cloud water droplet radius of zero?

@@ -196,7 +200,10 @@ def __init__(
ignore_day_of_year=False,
facular_sunspot_amplitude=None,
solar_variability_by_band=None,
aerosol_type='no_aerosol', **kwargs):
aerosol_type='no_aerosol',
Copy link
Member

Choose a reason for hiding this comment

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

You should add a description of what these additional keyword arguments do to the docstring of this method.

@JoyMonteiro
Copy link
Member

Thanks for the PR! I will go through the code this week, but I wanted to mention that the reason there
is a change at the highest model level is that we have commented out lines in the code

!swhrc(iplon,nlayers) = 0._rb
!swhr(iplon,nlayers) = 0._rb

which cause RRTMG to violate energy conservation. This is done in the LW code as well.

I have no idea why they decided to zero out these values. This might be ok for GCMs (which is where RRTMG is meant to be used) which have numerical dissipation in the upper model levels, but it used to
destabilise the single column model.

@codecov
Copy link

codecov bot commented Sep 11, 2018

Codecov Report

Merging #83 into develop will decrease coverage by 0.3%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #83      +/-   ##
===========================================
- Coverage    93.27%   92.96%   -0.31%     
===========================================
  Files           33       33              
  Lines         1324     1337      +13     
===========================================
+ Hits          1235     1243       +8     
- Misses          89       94       +5
Impacted Files Coverage Δ
climt/_components/rrtmg/sw/component.py 84.93% <66.66%> (-5.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 707909e...5d30e4c. Read the comment docs.

@JoyMonteiro
Copy link
Member

Thanks again for this work! I will review the code this week and wrap this up.

Have you updated HISTORY? You can add a comment to the top of HISTORY.rst to ensure we are
documenting all changes.

@JoyMonteiro
Copy link
Member

And before I forget, please also add your names to AUTHORS.rst!

@JoyMonteiro
Copy link
Member

This looks great!! I will be happy to merge this once you update HISTORY and AUTHORS.

@JoyMonteiro
Copy link
Member

Great! I'm going to ignore codedov. I will probably add some tests as well.
Thanks again @SallyDa @skosukhin, this is a very useful PR.

@JoyMonteiro JoyMonteiro merged commit f06adc6 into CliMT:develop Sep 17, 2018
This was referenced Sep 17, 2018
@SallyDa SallyDa deleted the McICA branch October 1, 2018 07:29
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

4 participants