Skip to content

Implement freq_at_index method for GenericSpectrogram (fixes #129)#138

Open
Monier-Ayman wants to merge 6 commits intosunpy:mainfrom
Monier-Ayman:freq-at-index
Open

Implement freq_at_index method for GenericSpectrogram (fixes #129)#138
Monier-Ayman wants to merge 6 commits intosunpy:mainfrom
Monier-Ayman:freq-at-index

Conversation

@Monier-Ayman
Copy link

PR Description

This pull request adds a new method freq_at_index(idx) to the Spectrum class (GenericSpectrogram).
The method returns the frequency corresponding to a given index on the frequency axis (freq_axis).

Motivation

Converting between indices and frequencies is useful for machine learning, data analysis, and other post-processing operations on spectrogram data.

Changes

  • Added freq_at_index(idx) in radiospectra/spectrum.py
  • Added tests for valid and out-of-bounds indices in radiospectra/tests/test_spectrum.py

Issue

Fixes #129

@Amityush-lgtm
Copy link

Hey @Monier-Ayman! Thanks for working on this. I’m new to radiospectra but I’m trying to learn by following along.
I noticed CI is still red on py313 (and pre-commit). I’m going to pull your branch and try to reproduce locally.
If you want, I can paste the exact failing traceback from py313 here once I have it, or suggest a small patch.

@Amityush-lgtm
Copy link

Hi @Monier-Ayman,

I pulled the branch and reproduced the CI failures locally. Two quick things:

In radiospectra/tests/test_spectrum.py, pytest is complaining that ValueError is too broad. You can fix it by adding the match argument:
Following this link: https://docs.astral.sh/ruff/rules/pytest-raises-too-broad/

Instead of :
with pytest.raises(ValueError): ...
Change it to:
with pytest.raises(ValueError, match="out of bounds"): ...

(use whatever string matches the actual error message)

pre-commit: looks like import sorting/formatting. Running this fixed it for me:

pre-commit run --all-files

One question on scope: this adds freq_at_index on Spectrum but issue #129 mentions GenericSpectrogram. Do you want the helper on GenericSpectrogram too, or is Spectrum the intended place now?

@Amityush-lgtm
Copy link

@Monier-Ayman Your latest fix is almost there! I think the reason it's still failing the test is that match="Index out of bounds" is looking for those words side-by-side. Since the error message is Index 5 out of bounds, the 5 breaks the match. If you change it to just match="out of bounds", it will pass.

@Monier-Ayman
Copy link
Author

Hi @Cadair and @samaloney, when you have a chance, could you please review this PR? It adds the freq_at_index(idx) method to GenericSpectrogram and includes tests for valid and out-of-bounds indices. Thanks!

@samaloney
Copy link
Member

@Monier-Ayman thanks for the PR but as I mentioned over on the issue #129 I'm not sure this is the best place to start as this will be core aspect of the upcoming GSOC project.

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.

Method that returns the frequency at a specific index on a Spectrogram

3 participants