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

Change Datasets model to models #2635

Merged
merged 11 commits into from Nov 30, 2019
Merged

Change Datasets model to models #2635

merged 11 commits into from Nov 30, 2019

Conversation

@cdeil
Copy link
Member

cdeil commented Nov 29, 2019

This PR is an attempt to fix #2102 by changing dataset.model to dataset.model on all our five dataset classes. If we do this change, we should do it now, before v0.15.

There's still two failing tests (see here) and I didn't update the tutorials or docs yet. There's merge conflicts already. Probably I'll have to re-start, but this PR shows that the change can be done within an hour.

As far as I understand it, our codebase already has migrated to this design that dataset.model should always be a SkyModels (plural) object. So IMO this change is the logical consequence, easier to understand for users. The logic is to accept for convenience different objects in init and in the setter (usually SkyModel, SkyModels and list of SkyModel), but then in the @models.setter to have checks and converters that guarantee that _models will always be a SkyModels, so everywhere where we access it (~ 1000 places) we can rely on that.

There's still the concern that we allow None and empty SkyModels list, which can cause issues (see here). We could either allow both and getters have to check on access. Or we only allow one in the setter.

@adonath @registerrier @QRemy - Thoughts?

If we go this way: should I rebase here and we try to push this in? Or should I re-start at a better time (e.g. Sunday evening)?

@cdeil cdeil added the cleanup label Nov 29, 2019
@cdeil cdeil added this to the 0.15 milestone Nov 29, 2019
@cdeil cdeil self-assigned this Nov 29, 2019
@cdeil cdeil added this to In progress in gammapy.modeling via automation Nov 29, 2019
@cdeil cdeil requested review from adonath, QRemy and registerrier Nov 29, 2019
gammapy/cube/fit.py Outdated Show resolved Hide resolved
@adonath

This comment has been minimized.

Copy link
Member

adonath commented Nov 29, 2019

Thanks @cdeil, I'm +1 to rebase. In #2632 I removed the e_true argument from the SpectrumEvaluator, so you have to adapt the changes here.

Concerning None vs. an empty SkyModels. I don't really care. Both works, I guess the empty models container leads to less code, but as long as we don't have something like SkyModels.append() it's barely useful for users and becomes and implementation detail...do whatever you prefer.

@cdeil cdeil force-pushed the cdeil:dataset.models branch from d102d20 to 6446851 Nov 29, 2019
@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Nov 29, 2019

@adonath - I've rebased and fixed all tests. I did not update the tutorials or docs yet.

I have to go now. If you can finish this up and merge it in, please do.
Otherwise, please leave a review and I'll try to do it tonight or tomorrow.

@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Nov 29, 2019

Just FYI: I would suggest to rename SkyModels -> Models tomorrow, and use that as a generic container, i.e. for special datasets like dark matter, they can add a model on the dataset anywhere they like with whatever parameters they like, and everything will work as long as they stick the models in dataset.models, it can be background or IRF or any other models.
Whether we manage to refactor BG models or not before v0.15 or v1.0, this would be my suggestion where to go.

@cdeil cdeil force-pushed the cdeil:dataset.models branch from 6446851 to 098fa54 Nov 30, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 30, 2019

Codecov Report

Merging #2635 into master will decrease coverage by <.01%.
The diff coverage is 90.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2635      +/-   ##
==========================================
- Coverage   91.56%   91.55%   -0.01%     
==========================================
  Files         141      141              
  Lines       15843    15883      +40     
==========================================
+ Hits        14506    14542      +36     
- Misses       1337     1341       +4
Impacted Files Coverage Δ
gammapy/cube/simulate.py 98.33% <ø> (+0.65%) ⬆️
gammapy/modeling/models/cube.py 94.69% <100%> (ø) ⬆️
gammapy/analysis/core.py 97.59% <100%> (ø) ⬆️
gammapy/modeling/serialize.py 98.86% <100%> (ø) ⬆️
gammapy/time/lightcurve_estimator.py 94.93% <66.66%> (ø) ⬆️
gammapy/spectrum/flux_point.py 91.02% <80.76%> (-0.26%) ⬇️
gammapy/spectrum/dataset.py 95.45% <94.11%> (-0.12%) ⬇️
gammapy/cube/fit.py 89.9% <94.28%> (-0.04%) ⬇️

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 3ddea70...b428ee5. Read the comment docs.

Copy link
Member

adonath left a comment

@cdeil I don't have time today do any review, please merge any time you like...

@adonath

This comment has been minimized.

Copy link
Member

adonath commented Nov 30, 2019

@cdeil I have some time now to work on this, I'll fix the remaining test fails and merge this PR after...

@adonath adonath merged commit 4f223a2 into gammapy:master Nov 30, 2019
9 of 12 checks passed
9 of 12 checks passed
greeting
Details
Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
codecov/patch 90.83% of diff hit (target 91.56%)
Details
codecov/project 91.55% (-0.01%) compared to 3ddea70
Details
Scrutinizer Analysis: 2 new issues, 7 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy #20191130.6 succeeded
Details
gammapy.gammapy (DevDocs) DevDocs succeeded
Details
gammapy.gammapy (Lint) Lint succeeded
Details
gammapy.gammapy (Test Python36) Test Python36 succeeded
Details
gammapy.gammapy (Test Windows36) Test Windows36 succeeded
Details
gammapy.gammapy (Test Windows37) Test Windows37 succeeded
Details
gammapy.modeling automation moved this from In progress to Done Nov 30, 2019
@adonath

This comment has been minimized.

Copy link
Member

adonath commented Nov 30, 2019

Thanks @cdeil!

@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Dec 1, 2019

@adonath - Thank you for finishing this up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants
You can’t perform that action at this time.