-
Notifications
You must be signed in to change notification settings - Fork 273
Add a StereoDispCombiner #2731
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
base: main
Are you sure you want to change the base?
Add a StereoDispCombiner #2731
Conversation
@@ -405,3 +415,251 @@ def predict_table(self, mono_predictions: Table) -> Table: | |||
stereo_table[f"{self.prefix}_telescopes"] = tel_ids | |||
add_defaults_and_meta(stereo_table, _containers[self.property], self.prefix) | |||
return stereo_table | |||
|
|||
|
|||
class StereoDispCombiner(StereoCombiner): |
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.
How does this algorithm differ from the implementation in EventDisplay by @GernotMaier ?
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.
also maybe a good citation: https://ui.adsabs.harvard.edu/abs/1999APh....12..135H/abstract, as this is essentially an implementation of "algorithm 3" of that paper.
if self.weights == "intensity": | ||
return data.hillas.intensity | ||
|
||
if self.weights == "konrad": |
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.
I think I've made this comment before: can we pick a better name here, since calling it "konrad" is not very helpful? This weighting is intensity x aspect-ratio, so we could call it "intensity-aspect-ratio" or "aspect-weighted-intensity" or "shape-intensity" or similar? maybe there is a shorter word... I think I prefer "aspect-weighted-intensity" , even though it's long, it is easy to understand.
- need to upload for different machine
- fix some minor bugs
- try implementing min distance calculation as ufunc
- gud lawd, thats a lot broadcasting
- should work for now, need some tests though
5f2dd49
to
47d2798
Compare
- Values are different, dont know why yet - But njit version is nearly twice as slow as broadcasting
47d2798
to
8902c7f
Compare
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.
We have a predicted disp.sign_score
that is not used in this method, which seems odd. Is that the reason why the pair-wise average including both disps is made rather than a simple weighted average over predictions? Is there citation for the MARS method, by the way? i'd be interested to see if simpler methods were tested.
for tel_id, dl2 in event.dl2.tel.items(): | ||
if dl2.geometry[self.prefix].is_valid: | ||
dl1 = event.dl1.tel[tel_id].parameters | ||
hillas_fov_lon = dl1.hillas.fov_lon.to_value(u.deg) |
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.
you don't actually have to convert or strip the units here, as astropy Quantities work automatically for numpy functions
>>> np.sin(90*u.deg) == np.sin(np.pi/2.0*u.rad)
True
>> np.sin(90*u.m)
UnitTypeError: Can only apply 'sin' function to quantities with angle units
That means you can avoid manually adding back the units later as well.
This |
It's also easy to argue why: especially at low energies, disp sign is not really reconstructible (random guess, sign_score ~ 0.5). But with multiple telescopes, it's quite easy to see which option is actually correct and make the weighted average over the correct locations. There are still a couple of different options for stereo disp, but most of them do not actually use the sign prediction, that is only used for mono as far as I know. The MAGIC MARS method is explained in a short paragraph here:
|
This is basically the StereoCombiner already implemented here
in
magic-cta-pipe
.It combines the mono DISP reconstruction by