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

Use aliases in perfmetrics #3058

Merged
merged 16 commits into from
Mar 21, 2023
Merged

Use aliases in perfmetrics #3058

merged 16 commits into from
Mar 21, 2023

Conversation

FranziskaWinterstein
Copy link
Contributor

@FranziskaWinterstein FranziskaWinterstein commented Feb 27, 2023

This pull request allow to use aliases in perfmetrics diagnostic. It further fixes a problem with doubled used index in zonal.ncl which in some cases lead to a too early break from a loop.

Description

I added the use of 'alias' when creating the annots for the plots. It is put front to be the preferred one if available.

There was further an issue with the index 'ii' in zonal.ncl. It could be overwritten by some function in interface_scripts/auxiliary.ncl . I changed it to 'modid' so that it is not overwritten anymore.

Checklist

It is the responsibility of the author to make sure the pull request 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 of pull requests:

@FranziskaWinterstein FranziskaWinterstein self-assigned this Feb 27, 2023
@FranziskaWinterstein FranziskaWinterstein marked this pull request as ready for review February 28, 2023 12:11
@schlunma
Copy link
Contributor

schlunma commented Mar 1, 2023

Hi @FranziskaWinterstein, thanks for this! We want to get this into v2.8, but need to do some tests and release a release candidate before we can have a look at that (~1 week). Hope this is okay for you!

In the meantime, could you please tick all the boxes in the pull request description? If something is not applicable, just remove it or tick it 👍

@FranziskaWinterstein
Copy link
Contributor Author

Hey @schlunma, thank you for having a look at it. Sure, take your time. I just found this was quite easy to do and put it in the pipeline right away. =)

@schlunma schlunma added this to the v2.8.0 milestone Mar 2, 2023
@valeriupredoi
Copy link
Contributor

hey @FranziskaWinterstein and @schlunma - we now have an RC1 for Core, with the environment gubbins set up in #3096 so please either merge that branch here or be on the ball to merge mian when that PR has been merged. BTW awesome work, and happy to see Franziska comes back to us with new contributions! 🍺

@valeriupredoi
Copy link
Contributor

hey @FranziskaWinterstein and @schlunma - we now have an RC1 for Core, with the environment gubbins set up in #3096 so please either merge that branch here or be on the ball to merge mian when that PR has been merged. BTW awesome work, and happy to see Franziska comes back to us with new contributions! beer

my apologies, I made a mistake in the comment - we have already setup the installation of Core RC1 for 2.8 in the main branch - so if @schlunma could merge that here then testing can go ahead merrily, I hope 🍺

@schlunma
Copy link
Contributor

Yes, will test now, just needed to wait for #3098 👍

@valeriupredoi
Copy link
Contributor

Hi @FranziskaWinterstein cheers much for your work here! Unfortunately I don't speak well NCL, so I'll let @axel-lauer do the technical/scientific review, and I can only have an overview of the testing, and merge when you guys ready - @remi-kazeroni is our release manager so he'll probably hurry you guys up so this gets included in the incoming v2.8 release 😁

@remi-kazeroni
Copy link
Contributor

Thanks for your contribution @FranziskaWinterstein! I leave the review and testing to @axel-lauer and @schlunma who know more than me on this topic. My plan is to finalize the release branch for the Tool late Monday or early Tuesday next week. It would be great to have this PR merged by then. Since this PR modifies shared diagnostics, I would prefer not to accept it after having started the next round of recipe testing on Tuesday. Please let me know on Monday if more time is needed to push this PR to the finish line 👍

revert changes
@FranziskaWinterstein
Copy link
Contributor Author

@axel-lauer The requested changes to the documentation are done. The changing of the loop index is reverted. Fine sofar?

Copy link
Contributor

@axel-lauer axel-lauer left a comment

Choose a reason for hiding this comment

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

Thanks @FranziskaWinterstein for taking into account my comments. This now looks all good to me.

Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

@schlunma schlunma merged commit 6faf263 into main Mar 21, 2023
@schlunma schlunma deleted the aliases_in_perfmetrics branch March 21, 2023 08:04
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.

8 participants