-
Notifications
You must be signed in to change notification settings - Fork 70
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
Option to output spectrally-decomposed TOA flux from RRTMG_LW #138
Conversation
Great start! I'll have some detailed feedback for you a little later. |
Sorry, the AMS meeting has sucked up all my attention this week but I haven't forgot about this! |
No problem! Hope it's going well! :) |
Some (maybe all) the test errors probably came from a failure of the data server that gets called upon first import of climlab. Try merging the latest master (which will fix an annoying problem with pytest warnings) and let the tests run again. |
…pectral_lw Merging new commits
Hi @brian-rose , I think I've merged the latest master. The CI tests are still failing though... |
When I run the tests locally I'm getting a lot of warnings due to ozone stuff:
This seems to be due to Update: O3 = O3source.interp_like(xTatm, kwargs={'fill_value':None}) to O3 = O3source.interp_like(xTatm, kwargs={'fill_value':'extrapolate'}) I think this is because you're only interpolating over 1d coordinates, and so |
@AndrewWilliams3142 I don't think those O3 issues would be causing the failures in It would be helpful if you put in a separate PR to fix the O3 issue, and we'll make sure that passes all test. |
…pectral_lw New commits, fixing ozone extrapolation
For the record, #140 fixed the O3 issue, and has now been merged into this PR. |
merging upstream changes
Codecov Report
@@ Coverage Diff @@
## main #138 +/- ##
======================================
Coverage ? 0.00%
======================================
Files ? 66
Lines ? 3562
Branches ? 0
======================================
Hits ? 0
Misses ? 3562
Partials ? 0 Continue to review full report at Codecov.
|
Hey @brian-rose , no problem at all. I think I've managed to do the changes you asked for? Tests are being triggered, we'll see if they pass.. :) |
Excellent, I'll have a careful look and see what needs doing in terms of documentation. |
climlab/radiation/rrtm/rrtmg_lw.py
Outdated
play, plev, tlay, tlev, tsfc, | ||
h2ovmr, o3vmr, co2vmr, ch4vmr, n2ovmr, o2vmr, | ||
cfc11vmr, cfc12vmr, cfc22vmr, ccl4vmr, emis, | ||
inflglw, iceflglw, liqflglw, cldfmcl, | ||
taucmcl, ciwpmcl, clwpmcl, reicmcl, relqmcl, | ||
tauaer) | ||
# Output is all (ncol,nlay+1) or (ncol,nlay) | ||
# Except for spectrally-decomposed TOA flux, olr_sr (ncol, nbndlw) | ||
if self.return_spectral_olr: | ||
self.OLR_sr = olr_sr + 0.*self.OLR_sr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested this with different grids? I'm guessing that olr_sr
will need to be reshaped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oooh good point, I've only tested for single-column models so far, do you mean to try with a 2d model?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes exactly. Climlab tries to support arbitrary sized lat-lon grids. The helper function _rrtm_to_climlab()
does the necessary reshaping. It can be a huge pain. In this case since there's an extra dimension in the output field it probably won't work out of the box.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @brian-rose ! I'm a bit confused by this. It seems like _rrtm_to_climlab()
just reverses the ordering of the pressure layers?
Also, maybe a dumb question but I didn't know climlab had any models with a longitude
component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, maybe a dumb question but I didn't know climlab had any models with a
longitude
component?
Not dumb at all. Support for lat-lon grids is more of an aspirational goal for climlab, and currently spotty at best, e.g. https://github.com/brian-rose/climlab/issues/86. I wasn't thinking clearly in my previous comment. For testing here, we should focus on 2D latitude-pressure grids
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks for the clarification! It works well with test_large_grid()
, which is a latitude-pressure grid, so is there more to do here? I don't think there's a need to call _rrtmg_to_climlab()
on olr_sr
because we don't want to flip the ordering of the last dimension
Taking a closer look at your code. This is pretty awesome! I just made one small change, removing the |
A couple of things I'm wondering about:
|
Thanks @brian-rose ! Good catch on the
|
It would be possible to implement both the upper/lower bounds and centers of each interval as I'll see if I can mock something up quickly. On the other hand, I don't think it's worth putting too much effort into these details because we are planning a complete rewrite of the climlab internals that will get rid of the |
Ok it wasn't that quick, because the climlab Easiest way for the user to access this would be using the
which produces
Ideally we would also wrap the spectral band boundaries into this object. You can access this information through the native climlab interface like
but the |
Just merged the latest updates from main branch, shouldn't cause any new failures. |
@AndrewWilliams3142 I'd like to advocate for changing |
Thanks so much for doing this @brian-rose ! Yes I couldn't get my head around how to implement this with the |
Fine by me! :) |
@AndrewWilliams3142 any interest in putting together a notebook with a demonstration of how to use this new feature? It would make a good addition to the tutorial collection at https://climlab.readthedocs.io/en/latest/tutorial.html |
Yep, I'd be down for that! Would it be easier to do this post-merge as a separate PR? I've done some stuff with the spectral OLR already in this repo (reproducing some of the Seeley+Jeevanjee 2020 paper on ECS) and so it would be easy to move that over :) |
Great stuff! I'll open a new issue for that and we can move the conversation over there. Thanks so much for your contribution here, really great new feature! |
This PR will close #137 (when finished).
The interface is the same as before, but if you want the spectrally-decomposed OLR outputted, then you pass
return_spectral_olr=True
toRRTMG_LW
when initialized.I've implemented a test which asserts that
assert np.allclose(simps(rcm.OLR_sr, rcm.RRTMG_LW_bands), rcm.OLR)
, after a 5-year RCE simulation.I'm a bit confused by why this test is failing at the moment, it says that
AttributeError: 'TimeDependentProcess' object has no attribute 'RRTMG_LW_bands'
, but as far as I can tell, I'm setting it as an attribute on line 56 ofclimlab/radiation/rrtm/rrtmg_lw.py
?I think that the FORTRAN is coded correctly, my only confusion is whether I should include the
delwave
factor in line 546 ofclimlab/radiation/rrtm/_rrtmg_lw/sourcemods/rrtmg_lw_rtrnmc.f90
. I'm not that familiar with how RRTM works so I'll have to defer to other on this.Thanks for the help so far, @brian-rose !
A