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

Reorganise llh.py #14

Open
robertdstein opened this issue Jan 22, 2020 · 3 comments
Open

Reorganise llh.py #14

robertdstein opened this issue Jan 22, 2020 · 3 comments
Labels
wish list Desireable but not urgent

Comments

@robertdstein
Copy link
Member

Is your feature request related to a problem? Please describe.
I'm always frustrated when coding in llh.py, because many things are nearly identical/redundant.

Describe the solution you'd like
A rationalisation, where the standard matrix method is used as default. Ideally, the 'standard' likelihood method would be removed entirely, but then things won't play nice with fitting_weights and flares. So it'd take some work.

Describe alternatives you've considered
Partially renaming things but maintaining separate implementations. It's easier in the short term, but more cumbersome to maintain.

Additional context
Flarestack will give nonsensical results if sources overlap under the default 'standard', but works fine with matrices. This is dangerous default behaviour. Changing names would probably lead to a change where old configs do not work anymore, or do different things.

@robertdstein robertdstein added the wish list Desireable but not urgent label Jan 22, 2020
@mlincett
Copy link
Collaborator

mlincett commented Jan 26, 2023

Issue #245 is likely related to what is reported in this issue.

It should be noted that llh.py lacks good documentation in general, as the distinction between 'standard', 'standard_overlapping' and 'standard_matrix' is not clear.

@robertdstein
Copy link
Member Author

So I think this is basically the way to go. For flare stacking and fitting weights, you need standard llh. This could perhaps be renamed to isolated_sources llh. Then for everything else, I think you need standard matrix, which basically does everything properly, and should perhaps be renamed full_matrix_llh or something like that. I think standard_overlapping is just a slower/original version of the matrix method, and is perhaps completely useless.

@robertdstein
Copy link
Member Author

I strongly favour giving everything a new name, so that old configs just break rather than working with completely different behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wish list Desireable but not urgent
Projects
None yet
Development

No branches or pull requests

2 participants