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
Plotting #241
Plotting #241
Conversation
…hey can be easily plotted from analysis results written to disk. Fix number of samples used in convergence plots (bootstrap histogram)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests are failing here. Can you show an example in pseudo-code of what you want to achieve? My original goal with this code was to have a separate handler for extended sources. Maybe we can coordinate and start working on that. I just have not had access to extended source analysis.
@henrikef thanks for these nice contributions. Maybe to make things easier on the testing/debugging you could divide this into two pull requests: one with the plotting things that do not use extended source (which I could merge right now since it is passing the tests), and another one with the addition that is failing now (the extended source part). This is just a suggestion, if you prefer to deal with everything at the same time that's fine too. In any case, please take care of adding tests for all functionalities introduced. Thanks! |
Codecov Report
@@ Coverage Diff @@
## master #241 +/- ##
=========================================
- Coverage 75.43% 75.14% -0.3%
=========================================
Files 94 94
Lines 10979 11058 +79
=========================================
+ Hits 8282 8309 +27
- Misses 2697 2749 +52 |
@grburgess has been working on the plotting interface up to now, I'll leave to him to review and merge when ready. In the meantime thanks to @henrikef for this contribution! |
@henrikef @grburgess what is the status on this? |
Hi everyone! I'd still like to get this pull request merged if possible. The travis-ci build is failing but as far as I can tell, the only test that fails is Regarding plotting extended source spectra: @grburgess , what was the motivation for having separate handlers for extended sources? Wouldn't it just mean a lot of code would have to be duplicated? Maybe I'm missing something, but I didn't see anything in the current code that was particular to point sources. Me and several colleagues are using 3ML to fit extended sources to HAWC data, and we need to be able to plot the spectra. So far, some people are using my fork/branch, but it's annoying to have to keep it up to date whenever there are other changes to 3ML. But if there's something wrong with my approach of using the exact same methods for extended sources as for point sources, please let me know! |
My main concern was having this be general enough so that things can be expanded upon and that plotting of point sources was different from extended sources. That being said, is it possible for you to provide an example of the usage so that we can see how it works? I trust that you and @giacomov have a much better handle on how extended sources should be plotted, so I really have to rely on your judgement to see if this fits into a bigger framework. Does this make sense? |
Maybe I'm being dense, but I don't understand what you mean by
Could you explain? I'm talking about plotting the 'total' flux here, not the surface brightness (flux per solid angle). I can't really upload HAWC datasets, and I think the fermipy plugin cannot handle extended sources yet, but I include an example with made-up results below:
|
Reading the discussion above I suggest these modifications:
I don't know all the details of the code, but I think these suggestions would avoid code duplication and at the same time allow for the possibility of plotting extended source spectra. |
Aha. Now I understand. I agree
/J. Michael
… On 25 Apr 2018, at 21:33, Giacomo Vianello ***@***.***> wrote:
Reading the discussion above I suggest these modifications:
plot_point_source_spectra -> plot_spectra
change the plot_spectra calling sequence to include two optional parameters ra and dec. Within the code we should check the type of the requested source, if it is a point source these parameters are ignored, if it is an extended source these parameters are mandatory. This would allow to plot the spectrum of the extended source at any position, so it will work both for extended source with the same spectrum everywhere (like in @henrikef example) and for sources where the spectrum changes point to point
after that, the only difference between a point source and an extended source would be in the calling sequence of the source. You would use spectrum = source(energies) for point sources, while spectrum = source(ra, dec, energies) for extended sources. This can be handled with a simple if. After this, all else is the same.
I don't know all the details of the code, but I think these suggestions would avoid code duplication and at the same time allow for the possibility of plotting extended source spectra.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Hi @giacomov and @grburgess ! So the issue is with extended sources where the spectrum depends on the RA and Dec? Could you maybe provide an example of that for me, as I've never really worked with them. Anyway, changing the name of the calling function is easy, I can do that :) Adding RA/Dec parameters less so. By default, the plotting function loops through all or a given subset of the sources in the model. So we have to think of a user-friendly way to supply the requested coordinates for extended sources then. After that, I really have no idea for how to get at the flux at a given point. I'll look into it but I'd appreciate some help. |
That would be great, what's the status here? |
Hi all, I'm sorry for falling behind on this PR. I am a bit behind on where things are. I think it would be very useful to have a skype between all of us to sort out what is the best approach as this will be very important for many instruments. |
While I understand the concern about more general sources where the spectral shape could depend on the location (is there implementation of this somewhere in astromodels?), I think what Henrike implemented here addresses 99% (or 100%) of the current need. |
So I looked into adding an option to plot the spectrum at a given direction as @giacomov had asked. I honestly couldn't figure out a good way to add it. But if someone else wants to help me, that'd be great! If not, and not having that option is a deal breaker, I'll try to keep this branch up to date with the 3ML master. As it is, it works great for plotting the total spectrum for sources with separable spatial and spectral components. |
Let me fix the issue with HAL and I'll get to this. There is only so much I can do in a given day ;-) The change to the plotting function that would allow plotting any source is trivial as explained above, I just need the time to sit down, do it and test it. Hopefully tomorrow... |
I propose some updates to plotting routines...
Added a function to joint_likelihood.py that will return 1D/2D likelihood profiles/contours for all parameters/combinations of parameters for convenience.
Added an option to plot_point_source_spectra() to also plot extended source spectra (this will just plot whatever was set as the spectral shape, i.e. the 'spatially integrated' spectrum.
3a. Moved convergence_plots() and plot_chains() from bayesian_analysis.py to analysis_results.py, so that the plots can be produced from analysis results loaded from disk.
3b. Changed convergence_plots() to use the same number of samples for the bootstrapping average as for the sliding window, I hope that was the original intent.