Skip to content

Fix #160: Improve default time axis formatting using ConciseDateFormatter#163

Open
Amityush-lgtm wants to merge 5 commits intosunpy:mainfrom
Amityush-lgtm:issue-160-concise-dataformatter
Open

Fix #160: Improve default time axis formatting using ConciseDateFormatter#163
Amityush-lgtm wants to merge 5 commits intosunpy:mainfrom
Amityush-lgtm:issue-160-concise-dataformatter

Conversation

@Amityush-lgtm
Copy link
Contributor

@Amityush-lgtm Amityush-lgtm commented Feb 24, 2026

PR Description

Fixes #160

This PR improves the default time axis formatting for spectrogram plots by introducing Matplotlib's ConciseDateFormatter and AutoDateLocator for more readable and concise time labels on the x-axis.

What Changed:

  • Introduced _TimeAxisMixin in radiospectra/mixins.py to share time axis setup logic between different plotting methods.
  • Converted self.times (astropy.time.Time) to native Python datetime objects before plotting. This allows Matplotlib to use its built-in ConciseDateFormatter and AutoDateLocatornatively.
  • Modified PcolormeshPlotMixin.plot() and NonUniformImagePlotMixin.plotim() to use the new mixin and datetime-based plotting.
  • Added tests to verify the formatter and x-label generation.

Visual Proofs:

test_wind_single_axes test_wind_shared_axes

AI Assistance Disclosure

AI tools were used for:

  • Code generation (e.g., when writing an implementation or fixing a bug)
  • Test/benchmark generation
  • Documentation (including examples)
  • Research and understanding
  • No AI tools were used

Regardless of AI use, the human contributor remains fully responsible for correctness, design choices, licensing compatibility, and long-term maintainability.

Note:

For the code given in the #58 (comment), there was a slight change made in the code for using datetime:

axes.set_xlim(Time("2020-07-11T02:15").datetime, Time("2020-07-11T02:50").datetime)

@samaloney, Let me know your thoughts on this! Happy to make changes.

@Amityush-lgtm
Copy link
Contributor Author

Amityush-lgtm commented Feb 26, 2026

I was unsatisfied with my current approach in using native datetime since we have to manually convert when using axes.set_xlim so i started exploring the other ways that avoids it. I am now confused between two alternatives to improve default time axis formatting without requiring manual user conversion.

  • First approach, Since you preferred to use time_support() from astropy and i started to find ways to keep it intact so for that i need to add an MJD aware ConciseDateFormatter adapter.
  • Second approach, to use Matplotlib date values self.times.plot_date internally and adding a small custom date converter so the axis can still accept astropy.time.Time.

@samaloney, Let me know your thoughts on this, which one should i prefer or any other alternatives you have. Happy to make changes!

@samaloney
Copy link
Member

Those plots look much better! So it sounds like there is an incompatibly between time_support and ConciseDateFormatter, it does seem like time_support takes some customisation support away.

the adapter approach sounds interesting would try to hook into the matplotlib.units API and remove the need to the time_support?

If you do try this just make sure to not remove the commits for this working approach above so it would be easy to compare.

@Amityush-lgtm
Copy link
Contributor Author

Amityush-lgtm commented Feb 26, 2026

@samaloney I have implemented the matplotlib.units approach. It works with both set_xlim(Time()) and set_xlim(Time().datetime). Let me know your thoughts on this.

@Amityush-lgtm
Copy link
Contributor Author

These are plots made using this method

verification_overlaid verification_subplots

@samaloney
Copy link
Member

I haven't forgotten this will have another look at this later this week.

Copilot AI review requested due to automatic review settings March 17, 2026 12:40
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses #160 by improving spectrogram time-axis labeling, switching the default x-axis formatting to Matplotlib’s date locator/formatter machinery for more readable, concise time ticks while preserving support for setting limits with both astropy.time.Time and datetime.

Changes:

  • Added a shared _TimeAxisMixin and a custom Matplotlib date converter to support astropy.time.Time inputs on the x-axis.
  • Updated PcolormeshPlotMixin.plot() and NonUniformImagePlotMixin.plotim() to plot using Matplotlib date-number values and apply AutoDateLocator + ConciseDateFormatter.
  • Expanded test coverage to validate date conversion behavior, axis limit compatibility, leap-second handling, formatter selection, and x-label content.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
radiospectra/spectrogram/tests/test_spectrogrambase.py Adds/updates tests asserting plot-date conversion, set_xlim compatibility (Time/datetime), leap-second handling, and ConciseDateFormatter usage.
radiospectra/mixins.py Introduces _TimeAxisMixin plus a Time-aware DateConverter, and updates plotting to use plot_date + concise time-axis formatting.
changelog/163.feature.rst Adds a news fragment describing the improved default time-axis formatting behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve the default time axis formatting

3 participants