Skip to content

Initial draft of SpectrogramMeta classes#245

Open
Amityush-lgtm wants to merge 9 commits into
sunpy:ndcube-refactorfrom
Amityush-lgtm:metadata-classes
Open

Initial draft of SpectrogramMeta classes#245
Amityush-lgtm wants to merge 9 commits into
sunpy:ndcube-refactorfrom
Amityush-lgtm:metadata-classes

Conversation

@Amityush-lgtm

@Amityush-lgtm Amityush-lgtm commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

PR Description

Fixes #243

Implementing a metadata class hierarchy for radiospectra, following the pattern used in sunraster and the LC Radio Workshop metadata requirements.
I tried to make some flowcharts as well, will attach them too.

Changes:

  • Added SpectrogramMetaABC and SpectrogramMeta (backed by NDMeta) in radiospectra/spectrogram/meta.py
  • Added WAVESMeta and CALISTOMeta instrument specific subclasses for initial setup
  • Updated GenericSpectrogram to wrap raw dict metadata into SpectrogramMeta automatically
  • Added tests for the new meta class

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.

This is an initial draft, so I've just tried to make the changes in WAVES and CALISTO, if it looks in the right direction then I'll add the remaining subclasses also. Let me know your thoughts on this @samaloney and @hayesla.

@Amityush-lgtm

Amityush-lgtm commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Sharing the flowchart i used while designing this:

class_heirarchy_diagram (5)

Comment thread radiospectra/spectrogram/meta.py Outdated
# Identification
@property
@abc.abstractmethod
def instrument(self):

@samaloney samaloney Jun 29, 2026

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.

I think a short description and the type info should be encode here for each property e.g.

def instrument(self) -> str:
    """
    Name of the instrument
    """
    pass

and then here if we only return str then the default south be "" if that better or worse then returning a str or none e.g. -> str|None

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

for properties like date_start, frequency_rangeor data_units, what should i write for them??

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.

"Start of the observation" Time
"Frequency range of observation" Quantity
"Unit for the data" Unit

Maybe we don't need the docstrings for them all but they types would be good

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

One more doubt, what would you prefer for observer_location and data_mask?

@samaloney

Copy link
Copy Markdown
Member

Also you can embed mermaid charts directly in GitHub doesn't match exactly the UML but close enough

classDiagram
    direction TB

    class NDMetaABC {
        <<ndcube.meta>>
    }

    class SpectrogramMetaABC {
        <<abstract>>
        +instrument
        +observatory
        +detector
        +date_start
        +date_end
        ...
    }

    class NDMeta {
        <<ndcube.meta>>
        dict subclass
        +axes
        ...
        +slice()
        +add()
        +rebin()
    }

    class SpectrogramMeta {
        <<concrete base>>
        dict-backed via NDMeta
        +instrument
        +observatory
        +detector
        +date_start
        +date_end
        ...
    }

    class WAVESMeta {
        +background
        +receiver
    }

    class CALISTOMeta {
        +observer_location
    }

    class OtherInstrumentMetas {
        <<...>>
    }

    %% Relationships
    SpectrogramMetaABC --|> NDMetaABC : inherits
    NDMeta --|> NDMetaABC : inherits

    SpectrogramMeta --|> NDMeta : inherits
    SpectrogramMeta ..|> SpectrogramMetaABC : implements

    WAVESMeta --|> SpectrogramMeta
    CALISTOMeta --|> SpectrogramMeta
    OtherInstrumentMetas --|> SpectrogramMeta
Loading

Comment thread radiospectra/spectrogram/meta.py Outdated
Comment on lines +84 to +88
@property
@abc.abstractmethod
def data_units(self) -> Unit:
"""Unit for the data"""
pass

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.

I think this is aleady cover by the unit in NDCube

Comment thread radiospectra/spectrogram/meta.py Outdated
Comment on lines +112 to +114
def observer_location(self):
"""Observer location."""
pass

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.

Maybe observe_coordinate would be better and this should be a SkyCoord | None

@Amityush-lgtm Amityush-lgtm Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got it, I'm working on it

@Amityush-lgtm

Copy link
Copy Markdown
Contributor Author

Also you can embed mermaid charts directly in GitHub doesn't match exactly the UML but close enough

classDiagram
    direction TB

    class NDMetaABC {
        <<ndcube.meta>>
    }

    class SpectrogramMetaABC {
        <<abstract>>
        +instrument
        +observatory
        +detector
        +date_start
        +date_end
        ...
    }

    class NDMeta {
        <<ndcube.meta>>
        dict subclass
        +axes
        ...
        +slice()
        +add()
        +rebin()
    }

    class SpectrogramMeta {
        <<concrete base>>
        dict-backed via NDMeta
        +instrument
        +observatory
        +detector
        +date_start
        +date_end
        ...
    }

    class WAVESMeta {
        +background
        +receiver
    }

    class CALISTOMeta {
        +observer_location
    }

    class OtherInstrumentMetas {
        <<...>>
    }

    %% Relationships
    SpectrogramMetaABC --|> NDMetaABC : inherits
    NDMeta --|> NDMetaABC : inherits

    SpectrogramMeta --|> NDMeta : inherits
    SpectrogramMeta ..|> SpectrogramMetaABC : implements

    WAVESMeta --|> SpectrogramMeta
    CALISTOMeta --|> SpectrogramMeta
    OtherInstrumentMetas --|> SpectrogramMeta
Loading

Thats so cool, I didn't know GitHub supported Mermaid diagrams. I'll definitely try using them, Thanks a lot!

@Amityush-lgtm

Copy link
Copy Markdown
Contributor Author

Should i add the rest of the classes in this PR or like break it in different PRs??

@samaloney

Copy link
Copy Markdown
Member

I think adding the classes necessary to handle the instruments that are currently supported would make sense.

@Amityush-lgtm Amityush-lgtm marked this pull request as ready for review July 3, 2026 19:03
@Amityush-lgtm

Copy link
Copy Markdown
Contributor Author

I have added the remaining classes, let know your thoughts on this @samaloney

@Amityush-lgtm Amityush-lgtm requested a review from samaloney July 3, 2026 19:06
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.

2 participants