Skip to content
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

Migrate detect.cwt to use gammapy.maps #1576

Merged
merged 5 commits into from Jul 30, 2018
Merged

Conversation

@hsiejkowski
Copy link
Contributor

@hsiejkowski hsiejkowski commented Jul 27, 2018

Not ready to merge, still some tests are failing.

@cdeil cdeil self-assigned this Jul 27, 2018
@cdeil cdeil added the cleanup label Jul 27, 2018
@cdeil cdeil added this to To do in gammapy.maps via automation Jul 27, 2018
@cdeil cdeil added this to the 0.8 milestone Jul 27, 2018
@cdeil cdeil changed the title Migrate detect.cwt to WcsNDMaps #1530 Migrate detect.cwt to WcsNDMaps Jul 27, 2018
@@ -429,7 +429,7 @@ class CWTData(object):
def __init__(self, counts, background, n_scale):
self._counts = np.array(counts.data, dtype=float)
self._background = np.array(background.data, dtype=float)
self._wcs = counts.wcs
self._wcs = counts.geom

This comment has been minimized.

@cdeil

cdeil Jul 27, 2018
Member

Can you call this _geom please, instead of wcs. Note that there is a geom.wcs, so if we don't distinguish between geom and wcs for Astropy WCS objects, it will be very confusing.

Also note that @adonath recently added a map.copy and geom.copy methods for convience.
You could use those here, especially importing and calling copy.deepcopy shouldn't be needed any more.

@registerrier registerrier moved this from To do to In progress in gammapy.maps Jul 28, 2018
@hsiejkowski hsiejkowski force-pushed the hsiejkowski:cwt_maps branch from 7aea666 to 0081270 Jul 30, 2018
@hsiejkowski
Copy link
Contributor Author

@hsiejkowski hsiejkowski commented Jul 30, 2018

This is the final version and is ready to review.

I will do the cleanup by storing the data members in Map objects in separate PR, since this requires many changes and it is not directly related to migration to gammapy.maps.

@@ -742,6 +742,10 @@ def get_region_mask_array(self, region):
pixcoord = PixCoord(idx[0], idx[1])
return region.contains(pixcoord)

def copy(self):

This comment has been minimized.

@cdeil

cdeil Jul 30, 2018
Member

@hsiejkowski - This copy method should be removed. It was already added on the base class.
To see it you have to rebase this branch against the latest upstream master.
Then you can remove this copy again, in a new commit (or squash your commits so that we don't go back and forth in the version history).

Could you please do that?

This comment has been minimized.

@hsiejkowski

hsiejkowski Jul 30, 2018
Author Contributor

I did the rebase, and the WcsGeom class still does not have copy()method, therefore I implemented it by myself. Should I move it to the parent class MapGeom?

This comment has been minimized.

@cdeil

cdeil Jul 30, 2018
Member

The method is in mastter:
https://github.com/gammapy/gammapy/blame/master/gammapy/maps/geom.py#L890

It was added in this commit 10 days ago:
b13cf3f

maybe you forgot to fetch the latest upstream master before doing rebase?

This comment has been minimized.

@hsiejkowski

hsiejkowski Jul 30, 2018
Author Contributor

I double checked and I have a fresh master. The commit you mention introduces a copy() method for MapCoord class, but self._geom2d/3d are instances of WcsGeom(MapGeom) class, which does not have copy().

This comment has been minimized.

@cdeil

cdeil Jul 30, 2018
Member

You're right. This branch is rebased against latest upstream master, and there is no geom copy.

Apologies!

I will merge this PR now, and then move your geom copy to the base class via a follow-up commit in master.

@@ -5,7 +5,7 @@
from ...utils.testing import requires_dependency, requires_data
from ...detect import CWT, CWTKernels, CWTData
from ...datasets import load_poisson_stats_image

This comment has been minimized.

@cdeil

cdeil Jul 30, 2018
Member

@hsiejkowski - the test still uses load_poisson_stats_image, which uses SkyImage, so we'd like to delete it.

Could you please update the tests to use Map to load the image file instead?

This should work:

def load_images():
    filename = '$GAMMAPY_EXTRA/test_datasets/unbundled/poisson_stats_image/counts.fits.gz'
    counts = Map.read(filename)
    background = counts.copy(data=np.ones(counts.data.shape, dtype=float))
    return dict(counts=counts, background=background)

I see there's several places below where the images are used. Probably we should use pytest.fixtures here, but possibly that's a bit complicated.

Maybe for now you just avoid the use of load_poisson_stats_image / SkyImage in the test somehow?

If you don't want to do this now, I can also do this in a follow-up commit tonight.

Apart from this and the geom copy mentioned in my last comment, I think this PR is ready to go.

This comment has been minimized.

@hsiejkowski

hsiejkowski Jul 30, 2018
Author Contributor

Done in e20da15

@cdeil cdeil merged commit 08e9ccc into gammapy:master Jul 30, 2018
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
gammapy.maps automation moved this from In progress to Done Jul 30, 2018
@cdeil
Copy link
Member

@cdeil cdeil commented Jul 30, 2018

@hsiejkowski - Thank you!

@cdeil cdeil changed the title Migrate detect.cwt to WcsNDMaps Migrate detect.cwt to use gammapy.maps Aug 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
gammapy.maps
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants