Fix plot_cxctime for compatibility with matplotlib 3.5

[jeanconn]

Description

plot_cxctime defined a default for fmt which was being passed through to the lower-level plotting functions that did the real work. However, this causes warnings to be issued by matplotlib when plot kwargs like linestyle or marker were passed, e.g. plot_cxctime(time, values, marker='o').

Interface impact

This changes the default line color from b (blue) to the matplotlib default of C0. This color depends on the selected style but by default will be a lighter shade of blue.

Testing

• [n/a ] Passes unit tests [No unit tests for this package]
• Functional testing

Functional testing

• flight https://icxc.cfa.harvard.edu/aspect/test_review_outputs/ska.matplotlib-pr24/plot_cxctime-plots-flight.html
• test https://icxc.cfa.harvard.edu/aspect/test_review_outputs/ska.matplotlib-pr24/plot_cxctime-plots-test.html

The plots and warnings look all as-expected between the two versions, except that flight seems to display a bug that color was previously ignored for errorbar plots. This behaves correctly in the PR.

(Note: the SHA tag in the test outputs does not match the commit here because of a subsequent squash of commits down to one commit).

Also used this interactively from IPython and found that it works as expected (with the fix from draw to draw_idle).

Fixes #22

[jeanconn]

The change to just use plot() might need more comprehensive testing than just #23, but seems to work.

[javierggt]

Do we prefer this over #23? (if all tests are ok)

[javierggt]

The functional tests above are for PR #23, not this one. At least that's what the URL says.

[taldcroft]

I think the functional test retest is for this PR since it gives the desired results (warnings only where expected). In general I prefer the approach here over #23.

[taldcroft]

@javierggt - I see you approved this, but the datetime comment I noted should be considered a blocker. It can badly impact memory and speed performance.

datetime comment above is a blocker.

[jeanconn]

I also just made this a pr against #23 but I suppose I can just change target.

[jeanconn]

And I'll rerun the testing in a new directory anyway for the suggested fixes and add something for 'tz'.

[taldcroft]

We do not need timezone support.

[jeanconn]

OK. So we cut that option too?

[jeanconn]

It seems like reviewing for removing 'tz' is again more work than testing it, but I don't have a strong opinion (has anybody used it for local time plots?).

[taldcroft]

Argh sorry, I didn't realize that there already was a tz option, I thought you were talking about adding it. No, don't cut the existing option. I just have no idea why that tz option was there originally, basically we never deal with time zones in CXC work.

[jeanconn]

Gotcha. No sweat. I just don't immediately see that it was actually doing anything before so I was trying to test that any changes around ax.axis_date(tz) were well-behaved.

[jeanconn]

Yeah, I still don't understand how 'tz' was being used to see if these changes break something related to it.

[taldcroft]

I pushed another commit that seems to work and generally simplifies the logic / code. I replicated the HTML test pages from @jeanconn and the plots all look good to me.

I discovered in playing around with this interactively that adding another line to the plot figure was not being reflected in the plot unless I called plt.show(). It turns out that command just does fig.canvas.draw_idle(), so I changed the corresponding line in plot_cxctime to use draw_idle instead of draw.

An important bit of functional testing that we'll need to check is with xija_gui_fit.

For this PR if this has converged then we should squash this down to one commit.

[jeanconn]

Sure. That's fine too. I just still haven't figured out what if anything ax.xaxis_date(tz) is doing.

[taldcroft]

I just still haven't figured out what if anything ax.xaxis_date(tz) is doing.

I'm not sure either and the code appears to work without it, but I also don't think it is hurting. Can you re-run your test notebook for the record?

[jeanconn]

Sure. PS I tried various values of 'tz' in flight and test and saw no differences, so was wondering if the xaxis_date() was intended for different kinds of date inputs for the original plot_date or some such, or if this had a different behavior in a much earlier version of matplotlib. But seems not worth more time today.

[jeanconn]

Test notebook outputs rerun for record (though I added the version to the output and it will have the 055c8c1 sha which won't quite match on rebase... no matter).

[taldcroft]

I updated the description to be more comprehensive since we will be pointing the community to this PR at a high priority.