Introduce map copy method

[adonath]

This PR introduces the Map.clone() method to simplify creating new maps from existing ones. This PR addresses #1547.

[cdeil]

Great, thanks!

Add a test that shows that the clone is an independent deep copy by default (using is not to test that object IDs are different)?

Is the complexity with init inspect needed? Or is there are different way to write this?

OK to merge if you want to move on, we can still adjust next week. But @woodmd and @registerrier - please review this.

[registerrier]

OK I might have missed the point of this clone method but I would not expect a clone method to be able to change the geometry of a Map.
Note also the possible issue with multi resolution HpxNDMap

[registerrier]

I am not sure I follow the logic here.
If you change the geometry you are not cloning the map. You can reproject, cutout but not clone, right?

I would simply return a ValueError if geom is in kwargs

[registerrier]

OK I see this is actually most of the usage for this clonemethod.
I think then this should be a private method. In my opinion, it is very confusing for a user to have a clone that basically can recreate a Map from scratch e.g. map.clone(geom=geom, data=data, meta=meta, unit=unit)

[registerrier]

Why did you remove the case for multi resolution map? Does this implementation work for healpix maps with mutliresolution scales?
Again the logic of cloning with a new geom is a bit strange to me. You just keep unit and meta. You don't gain much in terms of complexity with: map_out = self.__class__(geom=geom, meta=meta, unit=unit)

[registerrier]

Isn't it slower to use get_by_coord rather than get_by_idx used in the initial implementation?

[cdeil]

I see, the complexity comes from wanting to use clone in the methods that make maps with a different geom like sum_over_axes and cutout.

I think I agree with the comment by @registerrier :

If you change the geometry you are not cloning the map.

If possible, I would prefer to remove the complex implementation of clone with argument inspection. Is this possible?

import copy
def clone(self, data=None, meta=None, unit=''):
    if data is None:
        data = self.data.copy()
    if meta is None:
        meta = copy.deepcopy(self.meta)
    return self.__class(
        geom=self.geom.copy(),
        data=data,
        meta=meta,
        unit=self.unit,
    )

Note that str is immutable in Python, so making a copy is not needed / useful.

And then for the cases where geom changes, we keep using the from_geom.
Adding data=None there could help make the other cases nicer.

If so, it might be better to rename from_geom to make_map or something, but then we would break callers in Fermipy, so maybe leave the name for now?
https://github.com/fermiPy/fermipy/search?q=from_geom&unscoped_q=from_geom

[adonath]

I actually noticed the same issue, that the .clone() method should not change the geometry of the map. Unfortunately most of the internal usages do exactly that. Which is a bit of a pity, because we loose most of the elegance and simplification we wanted to introduce with the .clone() method. So I guess for those cases I'll change back to the old pattern with self.__class__() and just make sure, that I do it once correctly and uniformly. Do you agree @cdeil and @registerrier?

[adonath]

I've changed now to the following pattern. I introduced a hidden _init_copy() method, that is used to get a clean implementation of all the cases where again a Map object of the same type returned. In addition there is a public .copy() method, which forbids to change the map geometry. I must say I'm, still not happy with the pattern, but I don't want to spend any time further on this.

[cdeil]

I don't like the _init_copy in addition to copy, because then half of the time we'll use one and half of the time the other, i.e. devs have to learn both, for no good reason.
Also the _init_copy still does the __init__ argument inspecition, which I would like to avoid, again just for the reason "keep it simple", Gammapy code should be dead simple everywhere.

I'll make a follow-up commit here and then merge without further discussion, so that we can move on.

That doesn't have to be the final solution, we can re-discuss on Friday or later once we have more experience and callers with this, and then put something better if we find it.

But this should go in, IMO it's been a real pain-point and error-prone, to not have a way to make a copy without having to import and hard-code the map class (i.e. WCSNDMap or HPX map).