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

Update recipe_thermodyn_diagtool.yml: code improvements and more user options #1391

Merged
merged 67 commits into from
Jul 13, 2021

Conversation

ValerioLembo
Copy link
Contributor

@ValerioLembo ValerioLembo commented Oct 17, 2019

Description

This is a new version of recipe thermodyn_diagtool.yml

Main changes:



Before you get started

Checklist

It is the responsibility of the author to make sure the PR is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.

New or updated recipe/diagnostic:


To help with the number pull requests:

@valeriupredoi
Copy link
Contributor

@ValerioLembo awesome, Ill have a look at the code, but most importantly - does it work with the new fx vars implementation? Have you used #1272 as parent branch? 🍺

@ValerioLembo
Copy link
Contributor Author

ValerioLembo commented Oct 17, 2019

@valeriupredoi well... I included the sftlf_fx in a different way (l. 408 in thermodyn_diagtool.py) but I changed the recipe according to #1272 . And yes, it works!

@mattiarighi
Copy link
Contributor

It would make sense to merge this in #1272 and test everything there.
This is recipe is anyway the last to-do in #1272.

@bouweandela
Copy link
Member

It would make sense to merge this in #1272 and test everything there.
This is recipe is anyway the last to-do in #1272.

It might be better to keep it separate, as this pull request contains many changes so it may take longer to get it done.

@ValerioLembo
Copy link
Contributor Author

@bouweandela I am not actually planning to perform other changes to this pull request, so as far as I am concerned, it can be merged with version2_development.

@bouweandela
Copy link
Member

It looks like there are a lot of merge conflicts since #1272 got merged. Could you fix those please?

@ValerioLembo
Copy link
Contributor Author

Dear @Peter9192 @nielsdrost @valeriupredoi @bouweandela @zklaus

It's been a while since a scientific request has been asked for this long-awaited merging. Is there any chance we can get this done any time soon?

@valeriupredoi
Copy link
Contributor

we should (finally) get this in for the release in 2 weeks @zklaus - I think this recipe gets the prize for the longest wait, longer than Penelope 🤣

@ValerioLembo
Copy link
Contributor Author

@valeriupredoi is there a prize for that? ;)

@valeriupredoi
Copy link
Contributor

@valeriupredoi is there a prize for that? ;)

yes, a merge in main 😆

@Peter9192
Copy link
Contributor

Thanks for reminding us @ValerioLembo , I wholeheartedly agree! I'm just not sure who we can ask to do the scientific review. Giving it another try @ESMValGroup/atmosphere

@zklaus
Copy link

zklaus commented Jun 15, 2021

@axel-lauer, could you perhaps find a scientific reviewer for this?

@bouweandela
Copy link
Member

I have asked the co-authors of the GMD paper describing this diagnostic if they would be willing to do the scientific review.

@katjaweigel
Copy link
Contributor

I have started to look into it and can run both the new and the old version with the new core, but I can't promise how fast I could do a review (probably not today).

@ValerioLembo
Copy link
Contributor Author

Thanks @katjaweigel!

Copy link

@franklunkeit franklunkeit left a comment

Choose a reason for hiding this comment

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

I was asked to do a scientific review for this request, and I'm happy to do so.
I have to admit that I'm a bit biased as co-author of the respective paper, but I find the TheDiaTo package a very useful tool to evaluate climate models, and I'm very happy that it will be part of ESMValTool.

In my view, it is well documented (see minor below) and contains the appropriate references. However, I guess a user needs some experience to run it, and, in particular, to interpret the results, as it is common for such comprehensive diagnostics.
As far as I can see, all results (figures) produced by Niels in his test run (thank you for running it) are reasonable.

Thus, from my side I can support and approve the pull request.

As I never did such a review, I just tried out. I'm pretty sure that I did something wrong (or missed important points). E.g., I was ot able to check a checkbox for the review above, and did not find my name as a reviewer below. Please let me know, how to do it correctly.

@bouweandela: sorry, for presumably not being able to follow your instructions correctly.

Minor
In the paragraph for Lorenz energy cycle the last sentence:

'In order to account for possible gaps in pressure levels, the daily
fields of 2D near-surface temperature and horizontal velocities.'

seems somehow incomplete to me. You may like to have a look on this.

@ValerioLembo
Copy link
Contributor Author

Dear @franklunkeit, thank you for the review. While @bouweandela and the others decide on whether it is sufficient, I will change the documentation according to your suggestion...

@zklaus
Copy link

zklaus commented Jul 7, 2021

Thanks, @franklunkeit for the review. I don't think that your role as a co-author on the original paper is an obstacle here at all. After all, the paper was peer-reviewed, so the goal of the review here is not to establish the authority of the method, but rather to guarantee that the method as implemented in ESMValTool reflects the method as presented in the paper. Who could be better qualified to sign for that?

Thanks for addressing the documentation request swiftly, @ValerioLembo.

@bouweandela
Copy link
Member

Thanks a lot for taking the time to review @franklunkeit! I guess being able to check the boxes takes a bit more familiarity with the project, but from your comments I can see that the review addressed all the important points we expect from a scientific review.

@ValerioLembo If you would be able to improve the documentation as requested by @franklunkeit before Monday, we might still be able to include these updates in the imminent v2.3 release of the tool.

@zklaus
Copy link

zklaus commented Jul 13, 2021

@ValerioLembo, seeing as this seems to be only a one-sentence change, could you address it today? I am afraid otherwise we might need to bump it again to the next release 2.4.0.

@ValerioLembo
Copy link
Contributor Author

@zklaus Gosh! If this is the case, I will certainly do it this afternoon! It's definitely time for having this recipe out! 🤣

@zklaus
Copy link

zklaus commented Jul 13, 2021

@ValerioLembo, great. Looks like this is basically ready. All that's left is a bit of housekeeping. Could you please find the existing issue for this PR or create a new one to link to and double-check in the checklist above that all items are either ticked or struck out/removed?

@ValerioLembo
Copy link
Contributor Author

@zklaus Not sure if this was the right way to go for, but I did what you asked. I created a new PR, which is now available here: #2233 . I went through the checklist as well, and ticked everything.

@zklaus
Copy link

zklaus commented Jul 13, 2021

Thanks, @ValerioLembo, what you did was exactly right. Just to clarify the terminology: You didn't open a new PR (pull request), but rather a new issue. Issues contain descriptions and discussions of problems, PRs contain descriptions and discussions of solutions and are linked to the implementation of this solution by the corresponding branch.

Anyway, I edited the description at the top here to include the issue number and I think all is well now.

@zklaus zklaus merged commit a576e19 into main Jul 13, 2021
@zklaus zklaus deleted the version2_thediato_v2 branch July 13, 2021 15:59
@valeriupredoi
Copy link
Contributor

well done everyone, happy to see this finally merged 👴 😁

@ValerioLembo
Copy link
Contributor Author

ValerioLembo commented Jul 13, 2021

Awesome! And so it is gone now! 1 year, 8 months, 26 days since the PR was initially open: that's a major achievement in my short scientific career... It almost makes me dropping some tears!

@Peter9192
Copy link
Contributor

Thanks everyone for all the effort, and especially congrats to Valerio for pulling through! Happy to see this is PR finally merged. Hopefully the process will be smoother in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recipe: recipe_thermodyn_diagtool.yml (TheDiaTo)