Skip to content

Conversation

DanRyanIrish
Copy link
Member

PR Description

This PR introduces a new method NDCube.to_nddata, which allows for an NDCube to be converted to an NDData (or subclass of same) instance. The motivating use case is to enable users to easily drop coordinate-awareness from their data and thereby perform arithmetic operations with NDCubes.

Allow attributes to be changed via kwargs.
@DanRyanIrish DanRyanIrish added this to the 2.4.0 milestone Oct 6, 2025
@DanRyanIrish
Copy link
Member Author

@Cadair: I found time to PR this. Whatever we agree on the call signature, I think this captures the core of the implementation.

@DanRyanIrish
Copy link
Member Author

So @Cadair, what's your call signature decorator idea?

@DanRyanIrish DanRyanIrish requested a review from Cadair October 8, 2025 08:38
Copy link
Member

@wtbarnes wtbarnes left a comment

Choose a reason for hiding this comment

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

Once #880 is merged, a link in the docstring of this function to the narrative docs in that PR would be helpful in explaining the usefulness of this function as it is not immediately obvious how one would make use of this. This of course could be done in a subsequent PR as that PR depends on this one.

@DanRyanIrish
Copy link
Member Author

Once #880 is merged, a link in the docstring of this function to the narrative docs in that PR would be helpful in explaining the usefulness of this function as it is not immediately obvious how one would make use of this. This of course could be done in a subsequent PR as that PR depends on this one.

Sounds good. I can include this in #887

Comment on lines +1554 to +1559
if extra_coords is COPY:
extra_coords = copy.copy(self._extra_coords)
extra_coords._ndcube = new_nddata
new_nddata._extra_coords = extra_coords
if global_coords is COPY:
new_nddata._global_coords = copy.copy(self._global_coords)
Copy link
Member

Choose a reason for hiding this comment

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

We should really add these as kwargs to ndcube's constructor lol.

Copy link
Member Author

@DanRyanIrish DanRyanIrish Oct 9, 2025

Choose a reason for hiding this comment

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

I agree! Let's keep that as separate to this PR though.

Comment on lines +1545 to +1550
all_kwargs = {key.strip("_"): value for key, value in self.__dict__.items()}
all_kwargs.update(user_kwargs)
# Inspect call signature of new_nddata class and
# remove unsupported items from new_kwargs.
all_kwargs = {key: value for key, value in all_kwargs.items()
if key in inspect.signature(nddata_type).parameters.keys()}
Copy link
Member

Choose a reason for hiding this comment

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

I am not a fan of extracting all the attributes like this, it feels exceptionally brittle.

Do we really need to do this? Can't we just say if you need to copy custom attributes of a non-NDData class you need to specify them as explicit keyword arguments? Then this dictionary comprehension is just for the things where the kwarg value is COPY.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unless there's a better way to filter the kwargs by the call signature of nddata_type, then yes, we need this.

Copy link
Member

Choose a reason for hiding this comment

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

That wasn't the part I was objecting to. I'll open a followup PR when I get the chance.

@DanRyanIrish DanRyanIrish enabled auto-merge October 9, 2025 12:13
@DanRyanIrish DanRyanIrish disabled auto-merge October 9, 2025 12:13
@DanRyanIrish DanRyanIrish merged commit a3cc83c into sunpy:main Oct 9, 2025
20 checks passed
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.

3 participants