Improve plot_schedule_airmass() performance by drawing night bands only once

[michaelbaisch]

Fixes #539

[wtgee]

Hi @michaelbaisch, thanks for the PR. I’ll admit this is the first time I’ve looked at this code in detail, but I’m a bit confused. The night shading is already handled by plot_airmass, so it seems like plot_schedule_airmass could conform to that by simply passing the show_night variable into the plot_airmass call on line 493 and then removing the midnight plotting contained within plot_schedule_airmass.

Doing so produces the following plot:

@bmorris3?

Edit: FYI, I changed the color of the Transitions line to orange just because I was trying to figure out what was going on, ignore that change for this.

[wtgee]

I realized that the above might not handle a schedule that spans multiple days although if that’s the case maybe we should correct the functionality within plot_airmass.

[bmorris3]

Thanks for your PR @michaelbaisch!

I agree with @wtgee – I think it would be better to fix this in plot_airmass. Would you be interested in implementing a fix there?

[michaelbaisch]

Some thoughts: plot_airmass is called for each target in plot_schedule_airmass. So brightness_shading should only be set to True for one of those calls, otherwise the shading would be drawn multiple times again. I was using the := operator, which was added in python 3.8, but there is still a test for python 3.7, is that a hard dependency?

[bmorris3]

Some thoughts: plot_airmass is called for each target in plot_schedule_airmass. So brightness_shading should only be set to True for one of those calls, otherwise the shading would be drawn multiple times again.

Right, so it should be possible to set brightness_shading=True for only the first iteration in this loop that calls plot_airmass, right?

https://github.com/astropy/astroplan/blob/fda875841bded74a3e82daee8c85b002e56400c5/astroplan/plots/time_dependent.py#L492-L493

I was using the := operator, which was added in python 3.8, but there is still a test for python 3.7, is that a hard dependency?

I'm not sentimental about 3.7, but I'm not sure that one use of the walrus op is a sufficient reason to drop 3.7 support.

[bmorris3]

I'm not sentimental about 3.7, but I'm not sure that one use of the walrus op is a sufficient reason to drop 3.7 support.

This might force our hand though: https://numpy.org/neps/nep-0029-deprecation_policy.html#support-table

[michaelbaisch]

Not sure if I can do something about the failed "Test building of Sphinx docs (pull_request)"?

[bmorris3]

@michaelbaisch I'll investigate that failing test in a separate PR and follow up here when I sort it out. Thanks!

[bmorris3]

@michaelbaisch I've dug into it a bit, and I haven't sorted it out why this test fails on GitHub. It passes for me locally, so I'm going to merge and hope that #544 has already solved the problem. 🤞🏻

Thanks!

[michaelbaisch]

@bmorris3 why was this reverted?