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.

Sign up for GitHub

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

Jump to bottom

Group plots #116

Merged
merged 1 commit into from
Aug 14, 2021
Merged

Group plots #116

merged 1 commit into from
Aug 14, 2021

Conversation

charleskawczynski
Copy link
Member

@charleskawczynski charleskawczynski commented Aug 12, 2021 •
edited
Loading

This should help in reducing the number of clicks required to review all the results.

I think one more thing we could do is separate mse tables from what is plotted, as well as flexibility with, for example, plotting contours but not profiles.

This PR also fixed a bug in the variable map (updraft_qr)

@trontrytel
Copy link
Member

This looks great for the profiles. So nice to have everything in one place and to have the TC.jl main. Thank you so much for doing this!

Good luck with the contours!

@charleskawczynski charleskawczynski force-pushed the ck/group_figs branch 2 times, most recently from 26cd53f to 9ba7c42 Compare August 12, 2021 18:07
haakon-e
haakon-e approved these changes Aug 12, 2021
View reviewed changes
Copy link
Member

@haakon-e haakon-e left a comment

Choose a reason for hiding this comment

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

looks good. exactly where can I see all plots now?

integration_tests/utils/compute_mse.jl Show resolved Hide resolved
@charleskawczynski
Copy link
Member Author

looks good. exactly where can I see all plots now?

  • Click on details for the buildkite github action status (buildkite/turbulenceconvection-ci)
  • Click on a case (e.g., Bomex)
  • Click on Artifacts

There should be two figs per case: contours.png and profiles.png

@haakon-e
Copy link
Member

It seems like the contours needs to be reviewed.
image

Profiles looks reasonably good, but wouldn't hurt to have them a bit bigger, and also extend the figure boundary so all text is visible.
image

@charleskawczynski
Copy link
Member Author

Yeah, we definitely need some tweaking here

yairchn
yairchn approved these changes Aug 12, 2021
View reviewed changes
Copy link
Member

@yairchn yairchn left a comment

Choose a reason for hiding this comment

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

After fixing the plots I think its great to merge.

@charleskawczynski charleskawczynski force-pushed the ck/group_figs branch 2 times, most recently from af3bab2 to cf44759 Compare August 13, 2021 15:33
@charleskawczynski
Copy link
Member Author

Ok, we're now filtering out ["thetal_mean", "updraft_thetal", "u_mean", "v_mean", "qt_mean", "updraft_qt", "temperature_mean"], I think the plots look reasonable.

@jakebolewski
Copy link

It might also be useful to annotate the build with these plots: https://buildkite.com/docs/agent/v3/cli-annotate#embedding-and-linking-artifacts-in-annotations

trontrytel
trontrytel approved these changes Aug 13, 2021
View reviewed changes
Copy link
Member

@trontrytel trontrytel left a comment

Choose a reason for hiding this comment

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

Thank you!

@charleskawczynski
Copy link
Member Author

It might also be useful to annotate the build with these plots: https://buildkite.com/docs/agent/v3/cli-annotate#embedding-and-linking-artifacts-in-annotations

This PR is pretty big, but I'll take a look at this in a subsequent one.

I think this is as good as I can get it on the first pass. There's still room for improvement on the profiles.

@charleskawczynski
Copy link
Member Author

bors r+

@charleskawczynski
Copy link
Member Author

bors r-

@bors
Copy link
Contributor

bors bot commented Aug 13, 2021

Canceled.

@charleskawczynski
Copy link
Member Author

Actually, I'd like to see the remote results before merging.

@charleskawczynski
Copy link
Member Author

bors r+

@charleskawczynski charleskawczynski mentioned this pull request Aug 14, 2021
Closed
@bors
Copy link
Contributor

bors bot commented Aug 14, 2021

Build succeeded:

@bors bors bot merged commit c360990 into main Aug 14, 2021
@bors bors bot deleted the ck/group_figs branch August 14, 2021 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haakon-e haakon-e haakon-e approved these changes

@yairchn yairchn yairchn approved these changes

@trontrytel trontrytel trontrytel approved these changes

@szy21 szy21 Awaiting requested review from szy21

@ilopezgp ilopezgp Awaiting requested review from ilopezgp

@jiahe23 jiahe23 Awaiting requested review from jiahe23

Assignees
No one assigned
Labels
Continuous Integration ⚙️ Visualization 🎨
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@charleskawczynski @trontrytel @haakon-e @jakebolewski @yairchn