Conversation
jessiethw
left a comment
jessiethw
left a comment
There was a problem hiding this comment.
thanks for the new code @Samhori ! A few suggestions to make parts a bit clearer + some formatting. Overall looks very good!
One comment I made a couple times but not everywhere is to add docstrings to each function for the documentation. Most of the info you already included, but please format so that they appear correctly in the documentation when it's generated
| _float_index = not _fix_index | ||
| _index = 2.5 No newline at end of file | ||
| _index = 2.5 | ||
| _nside = 512 |
There was a problem hiding this comment.
this is a different nside than current. can you either make this a default in the posterior part of the code, or somehow else differentiate the nside you need? (giving it a new name also works)
There was a problem hiding this comment.
This can be whatever we want (changed for testing, will change it back), but we should probably talk about what value we want to send out. Multiple neutrinos can give resolutions much better than the floor for a single neutrino
| from . import sensitivity_utils | ||
| from . import plotting_utils | ||
| from .reports import FastResponseReport | ||
| from .reports import FastResponseReport,FastResponsePosteriorReport |
There was a problem hiding this comment.
add a space after the comma
| if livestream_start is None or livestream_stop is None: | ||
| livestream_start = self.start - 6. | ||
| livestream_stop = self.stop | ||
| print("livestream",livestream_start, livestream_stop) |
There was a problem hiding this comment.
is this for troubleshooting purposes? can probably put these in the if self._verbose or remove (same on ln 205)
| events = self.llh.exp | ||
| events = events[(events['time'] < self.stop) & (events['time'] > self.start)] | ||
|
|
||
| print(custom_events) |
There was a problem hiding this comment.
can you make this a bit clearer? I think it's a good idea to print any custom events injected, but can you give the user a message about what is being printed and the names (assuming this is the normal structured ndarray as events). something like
Custom events injected:
run, event, ra, dec
<array>There was a problem hiding this comment.
Would it make sense to move this somewhere else? It is in there for debugging for now, but I don't think this is the right place to print custom events if we want to keep that
|
|
||
| print(custom_events) | ||
| if(custom_events is not None): | ||
| events=np.concatenate([events,custom_events]) |
There was a problem hiding this comment.
if you're passing these separately in each function, can you maybe plot them with a different linestyle to show they are injected? maybe dashed or something, to make it clearer
There was a problem hiding this comment.
same comment in ln 619/637-638
|
|
||
| self.PosteriorSkymap=outputProbHPMap | ||
| self._make_posterior_fits() | ||
|
|
| temporal_weights = self.llh.temporal_model.signal(self.llh._events) | ||
| msk = spatial_weights * energy_ratio * temporal_weights > 10 | ||
| self.coincident_events = [] | ||
| if(custom_events!=None): |
There was a problem hiding this comment.
comment above about distinguishing injected events also applies here
| Phi[bins] = Phi[0] | ||
|
|
||
| return Theta, Phi | ||
| def plot_events2(dec,ra,sigmas, src_ra, src_dec, reso, sigma_scale=5., col = 'k', constant_sigma=False, |
There was a problem hiding this comment.
could you choose a more descriptive name here? just so it's clear what the difference between this and just plot_events is
There was a problem hiding this comment.
I think rather than adding these shell scripts, can you add the calls for two examples to the README at top level?
|
|
||
| f = CascadeFollowup(name, args.skymap, start, stop, skipped=args.alert_id) | ||
| t_mask=(f.llh.exp['time']<=f.stop)&(f.llh.exp['time']>=f.start) | ||
| print("__________________________________",f.start,f.stop) |
There was a problem hiding this comment.
looks like you have several messages printing by default - can you make sure that it's clear what each is when printed for the user to understand?
2 participants
Pull request for the development of the posterior skymap request. Currently, still some work to be done before we are ready to merge.