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

Multiple quantiles for angular_resolution #290

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

LukasBeiske
Copy link
Contributor

This makes it possible to pass multiple values to the quantile argument of pyirf.benchmarks.angular_resolution.
If done so, all the quantiles will be calculated and if only one quantile is passed (default), the output is the same as before.

quantile = [quantile]

if len(quantile) < 2:
keys = ["angular_resolution"]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's somewhat confusing to change the naming of the output key based on the number of quantiles. Just having a single valuye doesn't mean that that value is what should be defined as "angular resolution" (which is often defined as either 68% containment or 90% containment in different cases). Wouldn't it be better to just always call them containment_quantile_XX in all cases? or if you really want to be specific, something like angular_resolution_{value*100:.0f}pct_containment. That way there is no surprise to the user that the column names change.

I guess the issue is that somewhere else, the code looks for a magic column named "angular_resolution", but then it's also better to make that column name configurable, and have it default to the CTAO standard using the more explicit column naming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants