New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce map copy method #1554

Merged
merged 5 commits into from Jul 24, 2018

Conversation

3 participants
@adonath
Member

adonath commented Jul 20, 2018

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

@adonath adonath added this to the 0.8 milestone Jul 20, 2018

@adonath adonath self-assigned this Jul 20, 2018

@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.

@cdeil cdeil added this to To do in Map analysis via automation Jul 20, 2018

@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

Show outdated Hide outdated gammapy/maps/base.py
Show outdated Hide outdated gammapy/maps/hpxnd.py
def downsample(self, factor, preserve_counts=True):
geom = self.geom.downsample(factor)
coords = self.geom.get_coord()
vals = self.get_by_coord(coords)

This comment has been minimized.

@registerrier

registerrier Jul 20, 2018

Contributor

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

@registerrier

registerrier Jul 20, 2018

Contributor

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

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jul 21, 2018

Member

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

Member

cdeil commented Jul 21, 2018

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

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Jul 23, 2018

Member

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?

Member

adonath commented Jul 23, 2018

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

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Jul 24, 2018

Member

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.

Member

adonath commented Jul 24, 2018

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 cdeil assigned cdeil and unassigned adonath Jul 24, 2018

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jul 24, 2018

Member

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).

Member

cdeil commented Jul 24, 2018

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).

@adonath adonath merged commit b26b013 into gammapy:master Jul 24, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

Map analysis automation moved this from To do to Done Jul 24, 2018

@cdeil cdeil changed the title from Introduce map clone method to Introduce map copy method Aug 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment