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

Rename and refactor MapMakerObs #2450

Merged
merged 9 commits into from Oct 14, 2019
Merged

Conversation

@adonath
Copy link
Member

adonath commented Oct 10, 2019

This PR renames theMapMakerObs to MapDatasetMaker and refactors it to be used in the following way:

maker = MapDatasetMaker(geom=geom, offset_max=4 * u.deg)

for obs in observations:
    dataset = maker.run(obs)

This API change separates the data from the configuration of the maker, and allows to re-use the same object for multiple observations.

The new API makes it also possible to create e.g. a counts map only like so:

counts = Map.from_geom()
maker = MapDatasetMaker(geom=geom, offset_max=4 *u.deg)

for obs in observations:
    counts_obs = maker.make_counts(obs)
    counts.coadd(counts_obs)
@adonath adonath self-assigned this Oct 10, 2019
@adonath adonath added this to the 0.15 milestone Oct 10, 2019
@adonath adonath added the cleanup label Oct 10, 2019
Copy link
Contributor

registerrier left a comment

Thanks @adonath. Looks good.

The main question to me is whether defining the safe mask in the DatasetMaker is the right place. There might be other things to add than a max offset cut later on.

"""

def __init__(
self,
observation,
geom,
offset_max,

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 11, 2019

Contributor

If we move the max offset cut to a later step, it becomes useless to have it as an argument here.
Then cutout_size=5*u.deg could be the default and setting it to None would result in no cutout performed.

Or is it for a later change?

This comment has been minimized.

Copy link
@adonath

adonath Oct 11, 2019

Author Member

Maybe we wait for some more comments to #2451 and I make a follow up PR? But it would be definitely my preference to remove the mask safe computation too.

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 11, 2019

Contributor

OK. We need safe_mask anyway. So let's leave it there for now and change in a later PR.

cutout_kwargs = {
"position": observation.pointing_radec,
"width": 2 * self.offset_max,
"mode": "trim",

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 11, 2019

Contributor

Shouldn't the cutout mode be an argument?
If we are to perform ring background later on, we cannot use trim.

This comment has been minimized.

Copy link
@adonath

adonath Oct 11, 2019

Author Member

Sure, I can make it an argument. What was the reason again, that we cannot use partially contained observations for the ring background?

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 11, 2019

Contributor

yes, if the observation is partially contained you're likely to get an unexpected behavior.

This comment has been minimized.

Copy link
@adonath

adonath Oct 11, 2019

Author Member

Is there a more specific explanation, that I could put in the docstring? I guess technically it still works, but it comes with increased systematics at the edge of the image?

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 11, 2019

Contributor

Yes, it also increases the probability of having empty OFF pixels

This comment has been minimized.

Copy link
@adonath

adonath Oct 14, 2019

Author Member

I've added the cutout_mode option. I think general we should maybe re-discuss the default choice, so that we compute the whole observation by default. I can make a separate issue for this....

return psf

@lru_cache(maxsize=1)
def make_mask_safe(self, observation):

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 11, 2019

Contributor

OK. See the comment above regarding when to make the safe range.

This comment has been minimized.

Copy link
@adonath

adonath Oct 14, 2019

Author Member

Will be addressed in a follow-up PR...

@adonath adonath force-pushed the adonath:refactor_map_maker_obs branch from f7b43b0 to d5eb937 Oct 14, 2019
@adonath adonath merged commit 221bff8 into gammapy:master Oct 14, 2019
8 of 9 checks passed
8 of 9 checks passed
Codacy/PR Quality Review Hang in there, Codacy is reviewing your Pull request.
Details
Scrutinizer Analysis: 1 new issues, 14 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy Build #20191014.1 had test failures
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
@adonath

This comment has been minimized.

Copy link
Member Author

adonath commented Oct 14, 2019

Thanks for the review @registerrier!

@adonath adonath changed the title Rename and refactor `MapMakerObs` Rename and refactor MapMakerObs Nov 18, 2019
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.