search

CFMIP / COSPv2.0

COSP - The CFMIP (Cloud Feedbacks Model Intercomparison Project) Observation Simulator Package
41 stars 38 forks source link

Consistent use of hgt_matrix_half. Delete delz in warm rain diagnostics. #61

Closed alejandrobodas closed 3 years ago

alejandrobodas commented 3 years ago

This pull request addresses #60 and deletes variable delz, which was not used by the warm rain diagnostics.

RobertPincus commented 3 years ago

@alejandrobodas This looks slick, thanks for addressing this so quickly. Should we not add some documentation to explain where the half-levels are defined?

alejandrobodas commented 3 years ago

Hi @brhillman , please could you do the review for this fix? Basically, I've dropped the TOA level from hgt_matrix_half because it wasn't used. Now the array contains the bottom levels. I don't set the surface to 0 in cosp2_test for two reasons:

Thanks!

alejandrobodas commented 3 years ago

Hi @RobertPincus , I was writing my previous comment when you sent yours! Thanks for looking into this. Yes, that's a good idea, where do you think this should be documented? One option is in the cosp2_test driver, which is the template people will follow to implement it in models, but I'm open to suggestions.

RobertPincus commented 3 years ago

@alejandrobodas In the driver, yes, but wouldn't it also be important to note it in the COSP interface, where the input variables are explained?

alejandrobodas commented 3 years ago

@RobertPincus I've added comments in those two places as suggested. If you're happy, I'll wait to hear from @brhillman before merging.

alejandrobodas commented 3 years ago

@dustinswales ha, ha, no worries. Given that we flipped the order of levels between COSP1 and COSP2 and the added confusion of having two height arrays with different number of levels, you did an excellent job!

klein21 commented 3 years ago

Does it make sense to ask if someone has tested the fix in a GCM test case yet? @brhillmanhttps://urldefense.us/v3/__https:/github.com/brhillman__;!!G2kpM7uM-TzIFchu!nH9zwqUfU4jH2NFX_DOxLcigkU4IlkikQFxCNtNIofmXG44HphL2gBnm1SEEVkTL7Q$ @alejandrobodashttps://urldefense.us/v3/__https:/github.com/alejandrobodas__;!!G2kpM7uM-TzIFchu!n9jRsThK3aiZmjoko46P5nS6lPA-xs6ScgmxUM_KxQSIpJ2KEopFGJPCVfaTJYzg6g$

From: dustinswales @.> Reply-To: "CFMIP/COSPv2.0" @.> Date: Tuesday, June 15, 2021 at 7:41 AM To: "CFMIP/COSPv2.0" @.> Cc: Subscribed @.> Subject: Re: [CFMIP/COSPv2.0] Consistent use of hgt_matrix_half. Delete zlev in warm rain diagnostics. (#61)

@dustinswales approved this pull request.

@alejandrobodashttps://urldefense.us/v3/__https:/github.com/alejandrobodas__;!!G2kpM7uM-TzIFchu!nXPTiM9v6aMqcffTSzZIZjdDrOfIIjDoUJtcjUaGKxUV4X57NEUb2ekqlCEohKA4pw$ Thanks for cleaning up my mess so quickly! My apologies for this, I can't recall why I had done it this way.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://urldefense.us/v3/__https:/github.com/CFMIP/COSPv2.0/pull/61*pullrequestreview-684103481__;Iw!!G2kpM7uM-TzIFchu!nXPTiM9v6aMqcffTSzZIZjdDrOfIIjDoUJtcjUaGKxUV4X57NEUb2ekqlCHZJ97hWw$, or unsubscribehttps://urldefense.us/v3/__https:/github.com/notifications/unsubscribe-auth/AHZ364C3M4EWXLMSYXZWUSLTS5RCVANCNFSM46XHUQZQ__;!!G2kpM7uM-TzIFchu!nXPTiM9v6aMqcffTSzZIZjdDrOfIIjDoUJtcjUaGKxUV4X57NEUb2ekqlCFzOXwUEQ$.

alejandrobodas commented 3 years ago

Hi @klein21 , it would be helpful if someone could test it, but given that the change is simple and passes all the tests I don't think it is necessary to wait for a GCM test.

takmichibata commented 3 years ago

@alejandrobodas @dustinswales I apologize for this bug in warm rain diagnostics. Thank you very much for fixing it.

brhillman commented 3 years ago

@alejandrobodas sorry I missed the review on this! Thanks for addressing this!