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

Implement FluxEstimator #2810

Merged
merged 9 commits into from
Mar 6, 2020
Merged

Conversation

registerrier
Copy link
Contributor

Description

This pull request introduces the FluxEstimator.
So far result return is in the form of a dict. Do we prefer to have a Table row instead?
Dear reviewer

Some work still needed.
The early PR should help test with the new scipy 1.4.1.

@registerrier registerrier added this to In progress in gammapy.estimators via automation Mar 4, 2020
@registerrier registerrier added this to the 0.17 milestone Mar 4, 2020
@codecov
Copy link

codecov bot commented Mar 5, 2020

Codecov Report

Merging #2810 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2810      +/-   ##
==========================================
+ Coverage   92.17%   92.21%   +0.03%     
==========================================
  Files         147      148       +1     
  Lines       16636    16678      +42     
==========================================
+ Hits        15334    15379      +45     
+ Misses       1302     1299       -3
Impacted Files Coverage Δ
gammapy/estimators/parameter_estimator.py 87.05% <ø> (-0.16%) ⬇️
gammapy/estimators/__init__.py 100% <100%> (ø) ⬆️
gammapy/estimators/flux.py 100% <100%> (ø)
gammapy/modeling/models/cube.py 92.3% <0%> (+0.61%) ⬆️
gammapy/datasets/core.py 87.05% <0%> (+0.71%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 277ede6...d359c08. Read the comment docs.

adonath
adonath previously approved these changes Mar 6, 2020
Copy link
Member

@adonath adonath left a comment

Choose a reason for hiding this comment

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

Thanks @registerrier, I've left a few inline comments.

Minor: I think the _estimator suffix in the file name is not needed, as the file is already in the gammapy.estimators sub-package. Maybe introduce core.py and move the ParameterEstimator and FluxEstimator there?

gammapy/estimators/flux_estimator.py Outdated Show resolved Hide resolved
gammapy/estimators/flux_estimator.py Outdated Show resolved Hide resolved
gammapy/estimators/flux_estimator.py Outdated Show resolved Hide resolved
gammapy/estimators/flux_estimator.py Outdated Show resolved Hide resolved
@adonath adonath self-assigned this Mar 6, 2020
Copy link
Member

@adonath adonath left a comment

Choose a reason for hiding this comment

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

Thanks @registerrier, so far I have no further comments.

@adonath adonath merged commit 0276abd into gammapy:master Mar 6, 2020
gammapy.estimators automation moved this from In progress to Done Mar 6, 2020
@adonath adonath changed the title Flux estimator Implement FluxEstimator Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants