Initial base of GenericSpectrogram using NDCube#228
Conversation
Amityush-lgtm
changed the title
hayesla
added
the
NDCube Refactor
samaloney
left a comment
samaloney
left a comment
There was a problem hiding this comment.
This looks great!
Needs a change log I think you can use what in the PR description as a good start, also need to figure out the source of the test fail. If we know where it is can either add to list of ignored warning or fix
Co-authored-by: Shane Maloney <maloneys@tcd.ie>
|
I just applied a comment so you'll need to do a |
Should the changelog be marked as a feature or something else? |
|
I've added the changelog and marked it as a feature now will look at why the test is failing. |
|
I found the problem, the oldestdeps test is failing due to |
|
The CI tests are passing now. Let me know if any other changes are required @samaloney and @hayesla |
samaloney
left a comment
samaloney
left a comment
There was a problem hiding this comment.
This is a great first step, a few small suggestion mainly around documentation/docstrings.
| Base spectrogram class all spectrograms inherit. | ||
|
|
||
| Attributes | ||
| ---------- | ||
| meta : `dict-like` | ||
| Metadata for the spectrogram. | ||
| data : `numpy.ndarray` | ||
| The spectrogram data itself is a 2D array. |
There was a problem hiding this comment.
I think we need to keep/add the atts/parameters section.
| This keeps the existing ``GenericSpectrogram(data, meta)`` construction | ||
| pattern while the factory and source-specific metadata design is still | ||
| being worked out. The time and frequency arrays are used to construct the | ||
| WCS and are also retained in ``meta`` for compatibility with existing code. |
There was a problem hiding this comment.
I don't think this kind of narrative comment should be here it should be in the PR description or commit message etc
There was a problem hiding this comment.
Got it! Removing it
| super().__init__(data=data, wcs=wcs, meta=meta, **kwargs) | ||
|
|
||
| @classmethod | ||
| def from_arrays(cls, time, frequency, data, meta=None, **kwargs): |
There was a problem hiding this comment.
I don't think so its used currently, i'll remove it for now.
| @property | ||
| def time(self): | ||
| return self.axis_world_coords("time")[0] |
There was a problem hiding this comment.
More of a question but why are we adding a .time and the returning it from .times?
There was a problem hiding this comment.
I thought .times and .frequencies were still widely used so I kept them while making .time and .frequency the primary WCS-based properties. Should i remove them??
3 participants
PR Description
This PR takes a step toward the ideas discussed in #218 and #209 by making GenericSpectrogram an ndcube.NDCube subclass.
Key Changes:
GenericSpectrogramnow inherits from NDCube.build_spectrogram_wcsusingndcube.extra_coords.table_coordto automatically generate WCS from time and frequency metadata.GenericSpectrogramto use WCS-aware.timeand.frequencyproperties viaaxis_world_coords.ndcubetopyproject.toml.This keeps a stable foundation for using NDCube slicing and coordinate features while keeping the existing factory and mixins intact for now.
AI Assistance Disclosure
AI tools were used for: