Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Physical units in specshow axis labels #1819

Open
lostanlen opened this issue Mar 13, 2024 · 3 comments
Open

Physical units in specshow axis labels #1819

lostanlen opened this issue Mar 13, 2024 · 3 comments
Labels
display Plotting, visualization, matplotlib

Comments

@lostanlen
Copy link
Contributor

lostanlen commented Mar 13, 2024

Is your feature request related to a problem? Please describe.
Today i was looking at our (lesser-known!) gallery of advanced examples
https://librosa.org/doc/main/advanced.html
and noticed that the x-axis says "Time" without physical unit.

sphx_glr_plot_patch_generation_001

Meanwhile, the y-axis gives the physical unit (Hz) but not the quantity ("Frequency").
The same is true for tempograms. The axis says BPM not Tempo (BPM).

Describe the solution you'd like
It would be good to have consistency through the examples and documentation on how to label axes.

Describe alternatives you've considered
Calling xlabel, ylabel manually after specshow.

Additional context
Here's (an excerpt) of __decorate_axis:

def __decorate_axis(
    axis,
    ax_type,
    key="C:maj",
    Sa=None,
    mela=None,
    thaat=None,
    unicode=True,
    fmin=None,
    unison=None,
    intervals=None,
    bins_per_octave=None,
    n_bins=None,
):
    """Configure axis tickers, locators, and labels"""
    time_units = {"h": "hours", "m": "minutes", "s": "seconds", "ms": "milliseconds"}

    elif ax_type in ["tempo", "fourier_tempo"]:
        axis.set_major_formatter(mplticker.ScalarFormatter())
        axis.set_major_locator(mplticker.LogLocator(base=2.0))
        axis.set_label_text("BPM")

    elif ax_type == "time":
        axis.set_major_formatter(TimeFormatter(unit=None, lag=False))
        axis.set_major_locator(
            mplticker.MaxNLocator(prune=None, steps=[1, 1.5, 5, 6, 10])
        )
        axis.set_label_text("Time")

    elif ax_type in time_units:
        axis.set_major_formatter(TimeFormatter(unit=ax_type, lag=False))
        axis.set_major_locator(
            mplticker.MaxNLocator(prune=None, steps=[1, 1.5, 5, 6, 10])
        )
        axis.set_label_text("Time ({:s})".format(time_units[ax_type]))

Related: #1364 #1746

@lostanlen lostanlen added the display Plotting, visualization, matplotlib label Mar 13, 2024
@bmcfee
Copy link
Member

bmcfee commented Mar 14, 2024

This is a little subtle, but I think the behavior here is intended at least for time-like axes. From the code excerpt you cite, we do add physical units when they're explicitly specified. When the formatting is implicit and automatic, this can get dicey: would it make sense for ticks like 3:23 to be xlabeled as Time (sec)? I'd advocate for leaving this part as is, but I'm open to persuasion if there's a clear and consistent way to improve it.

The other part of your description, frequency-like axes, is probably worth looking into. On the one hand, I do think it's clear enough as is, and it's easy for a user to modify things after the fact if they want more detail. On the other hand, good default behavior is good. 😁

Frequency-like axes are not obvious, however. For example, pretty much anything we label with Hz can also be labeled as note or svara. Does it make sense to set the ylabel to Frequency (note) when using note ticks? This seems a little redundant and cluttered IMO; Note or Hz ought to be enough information. Given that, I think it is consistent to leave tempo axes as just units (BPM) and not Tempo (BPM).

That's all to say: I'm not sure what (if anything) we should do here. Curious for thoughts though.

@lostanlen
Copy link
Contributor Author

Thank you @bmcfee for having read me.

This is a little subtle, but I think the behavior here is intended at least for time-like axes. From the code excerpt you cite, we do add physical units when they're explicitly specified. When the formatting is implicit and automatic, this can get dicey: would it make sense for ticks like 3:23 to be xlabeled as Time (sec)? I'd advocate for leaving this part as is, but I'm open to persuasion if there's a clear and consistent way to improve it.

You guessed it: this came out while looking into #1746, in the context of visualizing long-duration bioacoustic signals.
I'd expect 4:33 to be labeled as Time (m).
If we leave it as is, the risk is to cause ambiguity between 4 minutes 33 seconds versus 4 hours 33 minutes. (Here's i'm assuming xlabel='time' as keyword argument, not h nor m, which would print the precise xlabel).

The other part of your description, frequency-like axes, is probably worth looking into. On the one hand, I do think it's clear enough as is, and it's easy for a user to modify things after the fact if they want more detail. On the other hand, good default behavior is good.

Thanks ... another frustration i often have is that there's no support for kHz, which is the preferred unit of bioacousticians (and some audio engineers?). Having yticks at 512 Hz, 1024 Hz, etc. is almost never what i want, and i'd like a quick way to set it to set it to 0.5, 1, 2, etc., kHz. But i guess that it's an entirely separate feature request.

Does it make sense to set the ylabel to Frequency (note) when using note ticks? This seems a little redundant and cluttered IMO; Note or Hz ought to be enough information.

For me, no label at all is fine in this case. I don't need a label to know that C5 is a note. I need a label to know that 4:33 is 4 minutes 33 seconds. Same logic for svara.

Given that, I think it is consistent to leave tempo axes as just units (BPM) and not Tempo (BPM).

Yes OK with just BPM. You're right, this one is perfectly clear as is, and that's an inconsistency i'm happy to live with.

Sorry for all the dissent and i hope we manage to resolve this!

@bmcfee
Copy link
Member

bmcfee commented Mar 18, 2024

You guessed it: this came out while looking into #1746, in the context of visualizing long-duration bioacoustic signals.
I'd expect 4:33 to be labeled as Time (m).
If we leave it as is, the risk is to cause ambiguity between 4 minutes 33 seconds versus 4 hours 33 minutes. (Here's i'm assuming xlabel='time' as keyword argument, not h nor m, which would print the precise xlabel).

Right, but the trouble here is that with automatic formatting, the units can change depending on the scale of the viewport, so the xlabel would have to be updated after limit changes. Doing this properly would require an axis callback. I'm not opposed to that in principle - we already do this for waveshow - but there's a problem with matplotlib currently that only one callback at a time per event is supported.

I do think our formatting is unambiguous though: 4:33 would always mean mm:ss because hour display would have three sections, right? This might change with datetime / #1364 though.

Thanks ... another frustration i often have is that there's no support for kHz, which is the preferred unit of bioacousticians (and some audio engineers?). Having yticks at 512 Hz, 1024 Hz, etc. is almost never what i want, and i'd like a quick way to set it to set it to 0.5, 1, 2, etc., kHz. But i guess that it's an entirely separate feature request.

That's a good point. I wonder if there's an elegant way to mix in frequency unit scaling with our existing axis formatters? Having a splat of fft_hz, fft_Khz, fft_Mhz (or whatever, and repeat for cqt, mel, etc) seems bad to me. I wonder if pint might be useful here?

For me, no label at all is fine in this case. I don't need a label to know that C5 is a note. I need a label to know that 4:33 is 4 minutes 33 seconds. Same logic for svara.

Agreed, though I think it's worth including for consistency (decorated axes always get labels).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display Plotting, visualization, matplotlib
Development

No branches or pull requests

2 participants