Fix mixed frequency unit plotting on shared axes

[Amityush-lgtm]

PR Description

Fixes a bug where plotting two instruments with different frequency units (kHz and MHz) on the same subplot would not understand the units and plot raw values.

Fixes #58

What changed

• Modified radiospectra/mixins.py to ensure frequency values are converted to the target axis unit if a mismatch is detected using a new helper function _frequency_values_for_axes.
• Added a regression test test_plot_mixed_frequency_units_on_same_axes in radiospectra/spectrogram/tests/test_spectrogrambase.py
• Verified that plotting MHz data on a kHz axis correctly scales the values.

Before and After

Before: (Using the given reproduction code)
In this, 13 MHz plots at y=13 on a kHz axis.

After:
With the fix, 13 MHz is correctly converted to 13,000 kHz.

Doubt

Is converting to kHz inside plot() the preferred behavior or is there any other way i should do it?
@samaloney, Let me know your thoughts on this. Happy to adjust!!

[samaloney]

This is not specific to this PR. I'm posting this to all open PRs. Please be aware of the update to our AI usage policy, specifically on the disclosure of and acceptable uses of AI.

[samaloney]

Thanks a bunch for the PR! I think the overall approach is great, but I’m not entirely convinced that adding custom attributes to the matplotlib axes object is the best way to go.

I think we should take a look at how astropys quantity_support should be able to use this or follow similar approach

[Amityush-lgtm]

Hi @samaloney, I've changed my approach based on your feedback. i switched to using quantity_support() from astropy and pass the frequency Quantity directly to matplotlib. Removed the custom _frequency_values_for_axes() helper and all the custom axis attribute logic also moved the test helper to conftest.py and fixed the test assertions. Let me know if this looks good!

[Amityush-lgtm]

I am facing a CI failure issue on this PR it is triggered because the older version of matplotlib in the oldestdeps environment uses pyparsing.parseString() which is deprecated (also present in #144)i am trying to fix this so far i added a targeted suppression for this specific warning in pytest.ini and its not working. Going to investigate a bit more. @samaloney , let me know your thoughts on this

[Amityush-lgtm]

I slightly changed the pyparsing warning suppression in pytest.ini. I also added a separate commit with some extra test coverage since the Codecov check were less. All CI checks are green now. Let me know if this looks good.

[samaloney]

Nearly there just need to use the quantity_support context manger rather than globally enabling e.g.

with quantity_support():
    ...

[samaloney]

LGTM once the CI passes thanks! (you can ignore the codecov check for this PR)

[Amityush-lgtm]

Thanks a lot for your review! @samaloney All CI checks are green now except the codecov/project.

[Amityush-lgtm]

@samaloney, i think i need to add a set_converter method to fix the oldestdeps CI failure for older matplotlib versions as they do not have axis.set_converter().

[samaloney]

I'm not sure what's happened but seem to have lost the quantity_support approach and all those commits? You should still have them locally and should be able to retrieve them with git reflog

[Amityush-lgtm]

My bad, while fixing merge conflict i accidentally reverted the approved quantity_support logic. I've pushed a fix that restores the proper quantity_support code. Thanks a lot for that git reflog command!

[samaloney]

No worries I only know because I've done the same thing 😆 I think this is good to go but I'll do a final review and merge etc on Monday

[samaloney]

Just few minor things and this should be ready to merge, thanks.

[samaloney]

@Amityush-lgtm thanks for sticking with this!

[Amityush-lgtm]

@samaloney, Thank you so much for your guidance and review. I really appreciate your support and learned a lot while working on this. Looking forward to contributing more.