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 __str__ method for Dataset #2287

Merged
merged 5 commits into from Jul 20, 2019
Merged

Conversation

@JouvinLea
Copy link

@JouvinLea JouvinLea commented Jul 16, 2019

This PR start implementation of str method for dataset class related to this issue (#2234)

I started with the fluxpoint dataset. Not sure about where to put the test. Once this is ok, we continue for the others dataset

@JouvinLea JouvinLea requested a review from adonath Jul 16, 2019
Copy link
Contributor

@registerrier registerrier left a comment

Hi @JouvinLea. Looks good to me.
What will happen if no model is set and the __str__ method is called?

Maybe this method could use different methods such as data_info() and fit_info() to print depending on the existence of a model attribute?

len(np.where(self.mask_fit == False)[0])
)
str_ += "\n"
str_ += "likelihood value : {}\n".format(len(self._likelihood))
Copy link
Contributor

@registerrier registerrier Jul 16, 2019

Can this fail if no model has been set?
Should there be an if statement to check for the presence of a model before trying to print the likelihood value?

@JouvinLea JouvinLea force-pushed the info_for_dataset branch from 2c4bd69 to af6e020 Jul 17, 2019
@cdeil cdeil added the feature label Jul 17, 2019
@cdeil cdeil added this to To do in gammapy.maps via automation Jul 17, 2019
@cdeil cdeil added this to the 0.13 milestone Jul 17, 2019
@JouvinLea
Copy link
Author

@JouvinLea JouvinLea commented Jul 18, 2019

@adonath @registerrier
Thanks for your comments! You can try it out for FluxDataset and MapDataset

@JouvinLea
Copy link
Author

@JouvinLea JouvinLea commented Jul 18, 2019

Don't try SpetrumDataset I will improve it now!

@JouvinLea JouvinLea force-pushed the info_for_dataset branch from a60a641 to 752fa9a Jul 18, 2019
@JouvinLea
Copy link
Author

@JouvinLea JouvinLea commented Jul 18, 2019

@adonath @registerrier
Everything implemented, you can do the final review

@cdeil cdeil changed the title __str__ method for Dataset Add __str__ method for Dataset Jul 19, 2019
@adonath
Copy link
Member

@adonath adonath commented Jul 20, 2019

Thanks @JouvinLea, I have made a few clean-up commits. Will merge now...

@adonath adonath merged commit 1f01f39 into gammapy:master Jul 20, 2019
3 of 9 checks passed
gammapy.maps automation moved this from To do to Done Jul 20, 2019
@adonath adonath changed the title Add __str__ method for Dataset Implement __str__ method for Dataset Jul 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
gammapy.maps
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants