DL3 Event List with EventPreprocessor#3004
Conversation
|
One thing of course missing is x_max, which we should have, but then we need to think about how to get th atmosphere profile, which would come from calibpipe, so I'll leave that for a future version. We have h_max, however, so can add that now, even if it's not in GADF |
x_max really is not that interesting at DL3 for gamma rays, is it? I wouldn't worry about filling that, we won't get good estimates for mono anyways. |
It's more for CR studies, but probably then you want DL2 anyhow (but maybe with event types...). |
35c993a to
4125df3
Compare
|
It would be nice to add a tutorial here, but I lack a nice test dataset of DL2 events with energy and gammaness applied and an optimized cuts file. We should create one, or else make a fixture to generate one. Maybe for another PR? |
| ) | ||
| icrs_coord = event_coord.transform_to("icrs") | ||
|
|
||
| return u.Quantity(np.column_stack((icrs_coord.ra.deg, icrs_coord.dec.deg)), u.deg) |
There was a problem hiding this comment.
Wouldn't it be better to return the SkyCoord object and then do coord.ra.to(u.deg) in the transform isntead of filling a 2d array and then indexing into that?
There was a problem hiding this comment.
It was just to output a simple 2D array, not a SkyCoord: astropy.table supports writing columns of SkyCoords, but our io.write_table function doesn't.
There was a problem hiding this comment.
But looking at the examples and the DL3 config, the goal seems to anyway be to get two separate columns? The 2d array is never actually used.
There was a problem hiding this comment.
it is used: it's a bit confusing maybe, but there is a 2D feature genereated with this function, and then the following two features extract the single columns. THat was the most efficient way to do this without calculating twice.
e.g.:
(
"reco_fov_coord",
"altaz_to_nominal(AZ, ALT, subarray_pointing_lon, subarray_pointing_lat)",
), # makes the 2D coordinate
("FOV_LON", "reco_fov_coord[:,0]"), # makes single column
("FOV_LAT", "reco_fov_coord[:,1]"), # makes single columnThe 2D column (reco_fov_coord) is then not included in the EventPreprocessor output, so it's temporary only.
There was a problem hiding this comment.
That's what I wrote above, right?
and then indexing into that
You can just leave the skycoord and then in the feature generator you access its fields instead of indexing into the array.
If you look in the #2979, I've implemented fixtures that generate the optimized cuts files from existing test files. |
| ("passed_gh", "apply_gammaness_cut(GAMMANESS, ENERGY)"), | ||
| ], | ||
| "quality_criteria": [ | ||
| ("VALID_RECO", f"{preprocessor.geometry_reconstructor}_is_valid"), |
There was a problem hiding this comment.
As quality criteria are independant for each reconstructor, I think it would be safer to perform an overall "and" on reconstructor quality cut.
There was a problem hiding this comment.
Yes, I will add criteria also for energy and gammaness. It's true they could be different, and it's useful to track
There was a problem hiding this comment.
I also realized I forgot the MULTIP cut, so I added a similar function hook where we can add a function of f(multiplicity, energy) that applies it. That should also come out of the OptimizationResult.
There was a problem hiding this comment.
Thinking more about it, but perhaps for the future after we refactor the optimization code to use this as well, would be to just copy over the OptimizationResult.quality_query directly to the QualityQuery here, rather than allowing the user to attach them - it's already stored in the output. That way we are sure to have the same selection applied.
I have to think what would be the cleanest way to do that, while keeping this class a bit generic. I think one option is to expand the OptimizationResult class to be a bit smarter: it should also handle the actual cutting, and then we can just pass it in like the Subarray, rather than require gammaness_cut_function and multiplicity_cut_function to be passed into the EventPreprocessor on reconstruction.
In that scenario, I would just have:
("PASSED_GH", "optimization_result.pass_gh(GAMMANESS, ENERGY)"),
("PASSED_MULTIP", "optimization_result.pass_multip(MULTIP, ENERGY)"
Great, then once that is merged, we can add a more fancy test here. Right now, I just pass in very simplisitic cut functions in the test, but eventually we can have an integration test that checks the whole thing. |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Also refactored the naming of intremediate columns, and added a hook where a complex multiplicity cut function can be passed in.
9d02316 to
b7a66e5
Compare
This adds a
FeatureSetto the EventPreprocessor for transforming DL2 to DL3 observed events. This is a work-in-progress to see what is needed to replace some of what is in #2979altaz_to_icrshelper function, and allow arbitrary objects to be passed into theFeatureGenerator(in this case the observatory location), and also allow the subarray to be passed in to init.DROP(cuts events) andMARK(creates new boolean columns for each quality query). The latter is useful for debugging or benchmarking.TODO:
add metadata to table→ better to do outside of this classExample:
Running with
mode="DROP"instead, gives this:And just to check it all worked:
the configuration it generates: