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

Refactor `MapFit` into `MapDataset` class #2026

Merged
merged 13 commits into from Feb 11, 2019

Conversation

Projects
None yet
2 participants
@AtreyeeS
Copy link
Member

AtreyeeS commented Feb 10, 2019

Hello @adonath and @registerrier

I am opening this PR now.
I am having some problem with locating some files (#2025), once that is sorted I will adapt analysis_3d as well. Perhaps you can do a review in the meantime.


def total_stat(self, parameters):
def likelihood(self, parameters):

This comment has been minimized.

Copy link
@AtreyeeS

AtreyeeS Feb 10, 2019

Author Member

Passing parameters here is unnecessary, but this is because there are too many callers to total_stat in gammapy.utils. I would prefer to clean up this caller in a follow up once this PR is merged (also needs cleanup in SpectrumFit and FluxPointDataset.

This comment has been minimized.

Copy link
@adonath

adonath Feb 11, 2019

Member

That's OK to leave it for now...I can do the clean up as well later.

This comment has been minimized.

Copy link
@AtreyeeS

AtreyeeS Feb 11, 2019

Author Member

Thanks! Ok to merge from my side then

@AtreyeeS AtreyeeS requested review from adonath and registerrier Feb 10, 2019

AtreyeeS added some commits Feb 10, 2019

@adonath adonath self-assigned this Feb 11, 2019

@adonath adonath added the cleanup label Feb 11, 2019

@adonath adonath added this to the 0.11 milestone Feb 11, 2019

@adonath
Copy link
Member

adonath left a comment

Thanks @AtreyeeS! This looks very good to me, I suggested a few minor changes.

)

@property
def stat(self):
def npred(self):

This comment has been minimized.

Copy link
@adonath

adonath Feb 11, 2019

Member

I think at the f2f meeting, we've decided that it's preferable to not make the npred an attribute, but rather a method (just in case it will accept additional parameters later). Could you do this change?

This comment has been minimized.

Copy link
@AtreyeeS

AtreyeeS Feb 11, 2019

Author Member

Good point! Changed


def total_stat(self, parameters):
def likelihood(self, parameters):

This comment has been minimized.

Copy link
@adonath

adonath Feb 11, 2019

Member

That's OK to leave it for now...I can do the clean up as well later.

@@ -947,6 +912,13 @@
"\n",
"* Analyse the second source in the field of view: G0.9+0.1 and add it to the combined model."
]
},
{

This comment has been minimized.

Copy link
@adonath

adonath Feb 11, 2019

Member

This is an empty notebook cell...maybe remove?

This comment has been minimized.

Copy link
@AtreyeeS

AtreyeeS Feb 11, 2019

Author Member

Done

]
},
{

This comment has been minimized.

Copy link
@adonath

adonath Feb 11, 2019

Member

Empty cell...maybe remove?

This comment has been minimized.

Copy link
@AtreyeeS

AtreyeeS Feb 11, 2019

Author Member

done

AtreyeeS added some commits Feb 11, 2019

@adonath adonath merged commit 3767b84 into gammapy:master Feb 11, 2019

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 5 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@adonath

This comment has been minimized.

Copy link
Member

adonath commented Feb 11, 2019

Thanks, @AtreyeeS!

@adonath adonath changed the title Adapt `MapFit` to `MapDataset` Refactor `MapFit` into `MapDataset` class Mar 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.