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

Implement `WcsNDMap.sample()` and remove `MapEventSampler` #2343

Merged
merged 6 commits into from Sep 10, 2019

Conversation

@fabiopintore
Copy link
Contributor

commented Sep 5, 2019

Following the PR in https://github.com/fabiopintore/gammapy/blob/event_sampling_pig/docs/development/pigs/pig-009.rst, the MapEventSampler class should be removed by the current gammapy.utils.random.inverse_cdf.py.

@adonath

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

Thanks @fabiopintore! As far as I can see this PR also includes the changes proposed in #2341. OK, if I only review this PR and we close #2341?

Copy link
Member

left a comment

Thanks @fabiopintore! I've left a bunch of inline comments, please address those, before we can merge.

@@ -688,3 +692,68 @@ def cutout(self, position, width, mode="trim"):
data = self.data[cutout_slices]

return self._init_copy(geom=geom, data=data)

def sample_position_energy(self, npred_map=None, n_events=None, random_state=0):

This comment has been minimized.

Copy link
@adonath

adonath Sep 6, 2019

Member

Please rename the method to .sample_coord() and remove the npred_map argument.

As we attach the .sample() method to the Map class, we can just access self.data and assume it contains an npred map already.

Parameters
----------
npred_map : `~gammapy.maps.Map`

This comment has been minimized.

Copy link
@adonath

adonath Sep 6, 2019

Member

Remove npred_map argument...

Sequence of coordinates and energies of the sampled events.
"""

self.random_state = get_random_state(random_state)

This comment has been minimized.

Copy link
@adonath

adonath Sep 6, 2019

Member

No need to define a new attributeself.random_state, just introduce a local variable random_state.

"""

self.random_state = get_random_state(random_state)
self.npred_map = npred_map

This comment has been minimized.

Copy link
@adonath

adonath Sep 6, 2019

Member

Again just remove npred_map...


self.random_state = get_random_state(random_state)
self.npred_map = npred_map
self.n_events = n_events

This comment has been minimized.

Copy link
@adonath

adonath Sep 6, 2019

Member

Again no need to define a new attribute self.n_events, just use the local variable n_events later...

coords = self.npred_map.geom.pix_to_coord(coords_pix[::-1])

# TODO: pix_to_coord should return a MapCoord object
geom = self.npred_map.geom

This comment has been minimized.

Copy link
@adonath

adonath Sep 6, 2019

Member

Just use self.geom here...


return MapCoord.create(cdict, coordsys=geom.coordsys)

def sample_events(self, npred_map=None, n_events=None, random_state=0):

This comment has been minimized.

Copy link
@adonath

adonath Sep 6, 2019

Member

Please just remove the sample_events() method. I think it's easy to create an astropy.table.Table object so no need for an extra method.

This comment has been minimized.

Copy link
@fabiopintore

fabiopintore Sep 7, 2019

Author Contributor

Thanks for the comment @adonath .
I've changed the code as you indicated.
About this only point, so do you mean to drop the Table object as output of the sample_coord() method and just stop it with:

` ....
axes_names = ["lon", "lat"] + [ax.name for ax in self.geom.axes]
cdict = OrderedDict(zip(axes_names, coords))
cdict["energy"] *= self.geom.get_axis_by_name("energy").unit

    return MapCoord.create(cdict, coordsys=self.geom.coordsys)`

?

This comment has been minimized.

Copy link
@adonath

adonath Sep 7, 2019

Member

Yes, I slightly changed my mind here. In the Map API we use MapCoord objects to handle coordinates and I thought it was more consistent to just have the sample_coord() method, which returns a MapCoord object. Returning a Table object would not be needed anymore at this point, hence there is no need for the additional sample_events() method either. Do you agree this makes sense?

This comment has been minimized.

Copy link
@fabiopintore

fabiopintore Sep 9, 2019

Author Contributor

Okay, I think it's reasonable hence it's fine to me to drop the sample_events() method. However, I was wondering if we should modify the PIG, in the code example here https://github.com/fabiopintore/gammapy/blob/event_sampling_pig/docs/development/pigs/pig-009.rst#mapdataseteventsampler .

This comment has been minimized.

Copy link
@adonath

adonath Sep 9, 2019

Member

I would say it's not needed. If we find small improvements during implementation, we just implement those. The PIG outlines the big picture and serves as a basis for discussion. It's not a 100% complete documentation of the development process.

n_events = rand_state.poisson(np.sum(npred.data))

nmap = WcsNDMap(eval.geom)
events = nmap.sample_events(npred_map=npred, n_events=2, random_state=0)

This comment has been minimized.

Copy link
@adonath

adonath Sep 6, 2019

Member

This can be simplified as npred.sample(n_events=2, random_state=0)


def test_map_sampling():
eval, npred = get_npred_map()
rand_state = get_random_state(0)

This comment has been minimized.

Copy link
@adonath

adonath Sep 6, 2019

Member

rand_state and n_events are not used in the test, just remove...

binsz=0.02,
map_type="wcs",
skydir=position,
width="5 deg",

This comment has been minimized.

Copy link
@adonath

adonath Sep 6, 2019

Member

Maybe decrease a bit the width here to e.g. 2 deg, so that the test runs faster...

@adonath adonath self-assigned this Sep 6, 2019
@adonath adonath added this to the 0.14 milestone Sep 6, 2019
@adonath adonath changed the title Remove the MapEventSampler class from gammapy.utils.random Implement `WcsNDMap.sample()` and remove `MapEventSampler` Sep 6, 2019
@adonath

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Thanks @fabiopintore, the PR looks much better now!

Unfortunately there is now a merg conflict. Can you please rebase against the latest master (git fetch origin and git rebase origin/master, while being in your feature branch, the force push to this PR) and fix the conflicts? Let me know if you need help with this...

@fabiopintore fabiopintore force-pushed the fabiopintore:invCDF_modified branch from b0323af to eba157a Sep 10, 2019
Copy link
Member

left a comment

Thanks a lot @fabiopintore! I've added two minor commits to fix an import statement and remove the default value for n_events. I have no further comments, once the CI is green I'll merge.

@adonath adonath merged commit ca47bd3 into gammapy:master Sep 10, 2019
8 of 9 checks passed
8 of 9 checks passed
Codacy/PR Quality Review Codacy was unable to analyse your pull request.
Details
Scrutinizer Analysis: 3 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy Build #20190910.8 succeeded
Details
gammapy.gammapy (DevDocs) DevDocs succeeded
Details
gammapy.gammapy (Lint) Lint succeeded
Details
gammapy.gammapy (Test Python36) Test Python36 succeeded
Details
gammapy.gammapy (Test Windows36) Test Windows36 succeeded
Details
gammapy.gammapy (Test Windows37) Test Windows37 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.