Skip to content

Conversation

@dopplershift
Copy link
Member

Description Of Changes

  • Fix test that didn't expect ax[hv]line to trigger unit-based axis labels.
  • Correct meteogram_metpy.py to adjust for deprecated/removed matplotlib arguments. Was pretty much unneeded.
  • Also fix above for removals pending for Matplotlib 3.8--past code wasn't even actually working correctly. Now this properly sets some spacing of the grid lines using Matplotlib's locators.

This also now modifies the nightly workflow to trigger on having a label applied to the PR, which will allow us to test PRs like this which need to run outside our standard config in order to validate the fix.

Checklist

@dopplershift dopplershift requested a review from a team as a code owner February 16, 2023 02:40
@dopplershift dopplershift requested review from dcamron and removed request for a team February 16, 2023 02:40
@dopplershift dopplershift added Type: Bug Something is not working like it should Type: Maintenance Updates and clean ups (but not wrong) Area: Examples Affects examples Area: Plots Pertains to producing plots labels Feb 16, 2023
@dopplershift dopplershift added this to the February 2023 milestone Feb 16, 2023
Matplotlib now triggers default unit-conversion axis labels for
axhline/axvline. Our test was not expecting that.
visible/b is implied when other keyword arguments are passed. Also, b=
is no longer supported with Matplotlib 3.7.
@dopplershift dopplershift added the nightly-ci Trigger a build of the nightly CI label Feb 16, 2023
@dopplershift
Copy link
Member Author

I guess an alternative to modifying the existing flow with reporting is to create a separate workflow that triggers on the label and doesn't have a reporting job. Not sure if that's worth the effort, but open to opinions from others.

I also just noticed these jobs are running on Python 3.10, so I'll try kicking these over to 3.11 when I patch this PR up to at least address the lint.

Passing 3 arguments to set_ylim() was not working as intended (with the
third argument being "step"). Instead, pass only two expected arguments
to set_ylim (was passing as emit, which ceases to be a positional
argument in Matplotlib 3.8) and set an appropriate tick locator instead
to work for "step".
@dopplershift dopplershift added nightly-ci Trigger a build of the nightly CI and removed nightly-ci Trigger a build of the nightly CI labels Feb 17, 2023
@dopplershift
Copy link
Member Author

So it looks like the current iteration requires the label to be reapplied. Not a huge fan, but not exactly wanting to write a bunch of API calls to avoid it...

Borrowed from one of Matplotlib's workflows.
@dopplershift
Copy link
Member Author

Ok, matplotlib had a version that showed me the right way to do this.

@dopplershift dopplershift removed the nightly-ci Trigger a build of the nightly CI label Feb 18, 2023
@dopplershift
Copy link
Member Author

Ok, so refactored so that we have:

  • a nightly builds workflow that triggers on cron, manual run, and pushes/prs that modify the workflow file--and opens an issue for any failures
  • A PR workflow that runs on PRs, but skips if we haven't added the appropriate label
  • Both of these run a "reusable workflow" that runs tests & doc builds using an unstable set of dependencies

@dcamron
Copy link
Member

dcamron commented Feb 21, 2023

I'm not sure how result propagates, so do

if ('${{ needs.Tests.result }}' === 'failure') {
const test_log = fs.readFileSync('tests-nightly.log', 'utf8').substring(0, 21000);
body += `The tests failed.\nLog:\n<details><pre>${test_log}</pre></details>`;
}
if ('${{ needs.Docs.result }}' === 'failure') {

need updated?

@dopplershift
Copy link
Member Author

Good catch. For one, I mistakenly left a condition on even kicking off the builds. Then it's definitely not requesting the results correctly. Probably gonna need some "prints" to figure out the right syntax here, assuming it's available.

@dopplershift
Copy link
Member Author

I think we're good to go here now.

@dcamron dcamron merged commit bef4e28 into Unidata:main Mar 1, 2023
@dopplershift dopplershift deleted the mpl-37 branch March 1, 2023 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Examples Affects examples Area: Plots Pertains to producing plots Type: Bug Something is not working like it should Type: Maintenance Updates and clean ups (but not wrong)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nightly build is failing

2 participants