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

Remove SpectrumResult object #2115

Merged
merged 5 commits into from Apr 17, 2019
Merged

Conversation

@adonath
Copy link
Member

@adonath adonath commented Apr 16, 2019

This PR removes the SpectrumResult object and attaches the corresponding plotting functionality to the FluxPointsDataset class.

@adonath adonath self-assigned this Apr 16, 2019
@adonath adonath added this to the 0.12 milestone Apr 16, 2019
Copy link
Contributor

@registerrier registerrier left a comment

Hi @adonath . This looks good to me. I have no relevant comment to make.

@@ -98,5 +98,5 @@ def run_fit(self, optimize_opts=None):

@property
def spectrum_result(self):
"""`~gammapy.spectrum.SpectrumResult`"""
return SpectrumResult(points=self.flux_points, model=self.fit.result[0].model)
"""`~gammapy.spectrum.FluxPointsDataset`"""
Copy link
Contributor

@registerrier registerrier Apr 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change from SpectrumFit to Fit(dataset) is for a future PR, I assume.

Copy link
Member Author

@adonath adonath Apr 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is going to be much more work. I'll try to make a few preparatory changes this week (see e.g. #2116).

ax.axhline(0, color="black", lw=0.5)
ax.set_ylabel("Residuals")
ax.set_xlabel("Energy ({})".format(self._e_unit))
ax.set_ylim(-1, 1)
Copy link
Contributor

@registerrier registerrier Apr 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix the y scale or leave it free to adapt to the data?

Copy link
Member Author

@adonath adonath Apr 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure, but I thought it's better to set a reasonable default, so that the standard plot always looks the same. Users can change it by setting ax.set_ylim() on the return axes objects outside the plotting method anyway.

@adonath
Copy link
Member Author

@adonath adonath commented Apr 17, 2019

Thanks for the review, @registerrier!

@adonath adonath merged commit 95d8cbf into gammapy:master Apr 17, 2019
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants