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
Addition of an ExtensionEstimator #4419
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Régis Terrier <rterrier@apc.in2p3.fr>
# find extension parameter | ||
model = datasets.models[self.source].spatial_model | ||
|
||
if hasattr(model, "sigma"): |
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 this code block should go into the estimate_size()
function instead and use the size parameter just as local variable.
Thanks @registerrier, this is very interesting and covers an important use case. As this is still a draft I just have a few preliminary comments:
|
@registerrier, should you update your branch such that the tests pass? |
Edges of the energy intervals where the extension is estimated. | ||
source : str or int | ||
For which source in the model to compute the extension. | ||
size_min : float |
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.
Maybe the default values should be added in the docstring...
|
||
def __init__( | ||
self, | ||
energy_edges=[1, 10] * u.TeV, |
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.
Maybe these default values should be removed... to be generic and to force the user to add edges
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.
Beyond the few comments on the docstring and initial values, it seems OK to be merged...
@registerrier can you update your code from the main? |
Description
This pull request proposes an
ExtensionEstimator
based on the parameter estimator.A possible improvement compared to the typical functionalities of the
ParameterEstimator
would be to handle a case where the extension parameter is kept fixed between all energies and then released to test the energy dependent morphology.Dear reviewer