Skip to content

Adding source for Nançay Decameter Array (NDA) observations#231

Open
Amityush-lgtm wants to merge 8 commits into
sunpy:mainfrom
Amityush-lgtm:add-nda-reader-source
Open

Adding source for Nançay Decameter Array (NDA) observations#231
Amityush-lgtm wants to merge 8 commits into
sunpy:mainfrom
Amityush-lgtm:add-nda-reader-source

Conversation

@Amityush-lgtm

Copy link
Copy Markdown
Contributor

PR Description

Fixes #226
This PR adds support for reading and interacting with solar radio burst data from the Nançay Decameter Array (NDA) in radiospectra.

Changes:

  • Added an NDASpectrogram class for handling NDA observations.
  • Added support for recognizing and loading NDA FITS files.
  • Extracted and attached relevant metadata such as frequency information and observatory details.
  • Registered the NDA spectrogram source so it works with the existing spectrogram factory.

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.

Working on the test, will push the tests in a while.

@Amityush-lgtm Amityush-lgtm changed the title Adding NDA reader source Adding source for Nançay Decameter Array (NDA) observations Jun 8, 2026

@samaloney samaloney left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this run for you locally?

Looks good just need to make sure the generic fits reader work with the NDA file and if not add a new branch

Comment thread radiospectra/spectrogram/sources/nda.py
Comment thread radiospectra/spectrogram/sources/nda.py Outdated
@Amityush-lgtm

Copy link
Copy Markdown
Contributor Author

Does this run for you locally?

Looks good just need to make sure the generic fits reader work with the NDA file and if not add a new branch

No, it's not working, I'm totally confused with how to fix this, that why i decided to open a PR first and get some inputs

@samaloney

Copy link
Copy Markdown
Member

So you can run the test with the python debugger pdb which should give you a console at the point of the error and you can step up/down the stack and evaluate variables etc or set a break point in your IDE

pytest --remote-data=any --pdb radiospectra/spectrogram/sources/nda.py

@Amityush-lgtm

Copy link
Copy Markdown
Contributor Author

Thanks for the tip! I'll try debugging it and see what i find.

@Amityush-lgtm

Copy link
Copy Markdown
Contributor Author

The reader failed because the filename has a .1 in v1.1.fits and NDA stores time and frequency in different columns than normal. I've tried to update the factory to handle both.

@samaloney samaloney left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok nearly there only one small fix left

Comment thread radiospectra/spectrogram/sources/nda.py Outdated
@samaloney samaloney added this to the 0.7 milestone Jun 10, 2026
@Amityush-lgtm Amityush-lgtm force-pushed the add-nda-reader-source branch from f3e649d to a73bf8f Compare June 10, 2026 10:11
@Amityush-lgtm Amityush-lgtm requested a review from samaloney June 10, 2026 16:51

@hayesla hayesla left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is looking good! one quick question, passing the file returns two spectrograms (RR, and LL) - do we have other classes that do the same thing?

thing to think about for the future on how we deal with this?

@Amityush-lgtm

Amityush-lgtm commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Yes, WAVES does the same thing returning one spectrogram per receiver (RAD1 and RAD2) from a single file and i think SoloRPW l2 does too.

@samaloney

Copy link
Copy Markdown
Member

Yea this was something I just did as it was easy - for this specific case the data array could be 3d, time, freq, polarisation as both the time and freq axis are identical.

More generally think like map we could have a keyword that would return either and SpectrogramSeqeunce (WAVES, WIND, RPW etc where the time axis is the same but freq axis is different) or SpectrogramCollection (if both axis are different).

Need to think about how to expose this to users and etc.

github-actions Bot and others added 4 commits June 11, 2026 15:23
@Amityush-lgtm Amityush-lgtm force-pushed the add-nda-reader-source branch from 1d2be1d to 98dff6e Compare June 11, 2026 09:57
@hayesla

hayesla commented Jun 11, 2026

Copy link
Copy Markdown
Member

yeah not a blocker on this PR just soemthing to think about. I think that these would be a NDCubecollection rather than a sequence

@Amityush-lgtm

Copy link
Copy Markdown
Contributor Author

is it normal for the py313-online CI job to take this long or does it have an issue??

@hayesla

hayesla commented Jun 11, 2026

Copy link
Copy Markdown
Member

I'm not sure whats going on with the tests

@Amityush-lgtm

Copy link
Copy Markdown
Contributor Author

some of the tests are rerunning quite a few times and still failing.

@hayesla

hayesla commented Jun 11, 2026

Copy link
Copy Markdown
Member

yes, i dont think any of the fails are related to this PR

@Amityush-lgtm

Amityush-lgtm commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

@samaloney might know about this test issue!

@samaloney

Copy link
Copy Markdown
Member

Yea I added retry and timeouts so the test can take bit longer but should pass more often. Do they run locally buy ruing tox in the root of repo (you might have to pip install tox)

@samaloney samaloney left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to go

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.

Add source for Nançay Decameter Array (NDA) observations

3 participants