Simplify NDCube.to_nddata

[Cadair]

I couldn't take it.

[Cadair]

My only remaining question is if we want to add a copy=False kwarg here which changes the copy behaviour to be copy by value rather that the default of copy-by-reference?

[DanRyanIrish]

I'm afraid that, as currently written, I don't see this as an improvement to what has already been merged for a few reaons.

First, we can't use bool to represent attributes that need to be copied because True is a valid user-defined value for mask.

Secondly, what we currently have allows users to convert to an NDCubeBase subclass while carrying over extra_coords and global_coords. I can imagine this being useful, especially if users want to change a value during the conversion, e.g. uncertainty.

Thirdly, if to_nddata is being used between NDCubeBase subclasses with additional attributes/kwargs, then this line that has been removed in this PR, captures those. By removing it, this code now unnecessarily would cause such cases to break. Alternatively, it unnecessarily requires maintainers of NDCubeBase subclasses to over-ride this method.

Additionally, I don't understand the depth of the objection to the to_nddata method. I agree that it's not the most elegant philosophically speaking. But as far as I can see, it is the most elegant practical solution to a problem that our users need solved.

[DanRyanIrish]

We can't use mask=True here as True is a valid user input for mask.

[Cadair]

Oh, that's so freaking stupid.

[Cadair]

The solution to this is that the one user who wants to do this uses np.True_ rather than True to set the whole mask to be a single boolean.

[DanRyanIrish]

Suggested change

	user_kwargs = {key: getattr(self, key) if value is True else value for key, value in user_kwargs.items()}
	user_kwargs.update(kwargs)
	nddata_sig = inspect.signature(nddata_type).parameters.keys()
	user_kwargs = {key: value for key, value in user_kwargs.items() if key in nddata_sig and value is not COPY}
	all_kwargs = {key.strip("_"): value for key, value in self.__dict__.items() if key in nddata_sig}
	all_kwargs.update(user_kwargs)

[Cadair]

First, we can't use bool to represent attributes that need to be copied because True is a valid user-defined value for mask.

eugh, I still don't think breaking this truly weird case is worth the extra complexity.

Secondly, what we currently have allows users to convert to an NDCubeBase subclass while carrying over extra_coords and global_coords. I can imagine this being useful, especially if users want to change a value during the conversion, e.g. uncertainty.

It doesn't break this or any other use cases it just means you have to opt-into copying these things. See the test and the new example in the docstring.

Thirdly, if to_nddata is being used between NDCubeBase subclasses with additional attributes/kwargs, then this line that has been removed in this PR, captures those. By removing it, this code now unnecessarily would cause such cases to break. Alternatively, it unnecessarily requires maintainers of NDCubeBase subclasses to over-ride this method.

see above.

Additionally, I don't understand the depth of the objection to the to_nddata method. I agree that it's not the most elegant philosophically speaking. But as far as I can see, it is the most elegant practical solution to a problem that our users need solved.

I think we are conflating too many tasks into one method which makes the API and the implementation too complex both for us to maintain and for users to understand. I think we need to de-scope this thing. I'd almost rather just get rid of nddata_type completely at this point.