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

Move Analysis to new gammapy.analysis #2528

Merged
merged 2 commits into from Nov 12, 2019
Merged

Move Analysis to new gammapy.analysis #2528

merged 2 commits into from Nov 12, 2019

Conversation

@Bultako
Copy link
Member

Bultako commented Nov 11, 2019

This PR renames scripts module into analysis everywhere in the gammapy code base, checks for regression tests and documentation building have been made.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 11, 2019

Codecov Report

Merging #2528 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2528      +/-   ##
=========================================
+ Coverage    91.5%   91.5%   +<.01%     
=========================================
  Files         146     147       +1     
  Lines       16727   16728       +1     
=========================================
+ Hits        15306   15307       +1     
  Misses       1421    1421
Impacted Files Coverage Δ
gammapy/analysis/core.py 94.16% <ø> (ø)
gammapy/scripts/__init__.py 100% <ø> (ø) ⬆️
gammapy/analysis/__init__.py 100% <100%> (ø)

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 4379f04...d27e05a. Read the comment docs.

@adonath adonath self-assigned this Nov 11, 2019
@adonath adonath added this to the 0.15 milestone Nov 11, 2019
@adonath adonath added the cleanup label Nov 11, 2019
@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Nov 11, 2019

@adonath @registerrier - Do you think we should make this name change or not?

I'm 50:50, OK either way.

@cdeil cdeil added this to In progress in gammapy.analysis via automation Nov 11, 2019
@adonath

This comment has been minimized.

Copy link
Member

adonath commented Nov 11, 2019

@cdeil @Bultako I remember, that we talked about this during one of the dev meetings, but I think we did not take a decision.

I slightly prefer gammapy.analysis as a namespace for the Analysis class, but what I don't like is that the cli scripts are in the same sub-package. So I would even propose to introduce a separate gammapy.analysis sub-package, copy over the Analysis and AnalysisConfig class and keep the cli in gammapy.scripts.

@Bultako

This comment has been minimized.

Copy link
Member Author

Bultako commented Nov 11, 2019

Yes, it's true no decission has been taken on renaming the package so far, though it has been evocated several times. But I think it's better to take a decission while it's still not very hard to do it (not much code in the high-level interface), users are still not used to work with it, and the big work on documentation has not yet started. With this PR I kind of force to take the decission now, sorry :)

I slightly prefer gammapy.analysis as a namespace for the Analysis class, but what I don't like is that the cli scripts are in the same sub-package. So I would even propose to introduce a separate gammapy.analysis sub-package, copy over the Analysis and AnalysisConfig class and keep the cli in gammapy.scripts.

I'm +1 to do that.

@registerrier

This comment has been minimized.

Copy link
Contributor

registerrier commented Nov 11, 2019

I slightly prefer gammapy.analysis as a namespace for the Analysis class, but what I don't like is that the cli scripts are in the same sub-package. So I would even propose to introduce a separate gammapy.analysis sub-package, copy over the Analysis and AnalysisConfig class and keep the cli in gammapy.scripts.

I'm +1 to do that.

I am +1 too.

@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Nov 11, 2019

I'm also OK to just create gammapy.analysis, and to keep gammapy.scripts for the scripts.
Please either do that from scratch in a new PR, or if you do it here, please squash commits to avoid moving things back and forth and losing version history connect.

With this PR I kind of force to take the decission now, sorry :)

That's perfectly fine. I think usually an issue with a suggestion and waiting for comments for a day is more effective, but if the work to make a PR isn't very high, just bringing the PR is a good way, as long as you're willing to take the risk to have it rejected or having to change it significantly like in this case.

move high-lelve interface code into analysis sub-package
@Bultako Bultako force-pushed the Bultako:hli branch from 338bd19 to 89aaf2c Nov 11, 2019
@Bultako

This comment has been minimized.

Copy link
Member Author

Bultako commented Nov 11, 2019

or if you do it here, please squash commits to avoid moving things back and forth and losing version history connect.

ok, it's done.

Copy link
Member

cdeil left a comment

Please rename gammapy/analysis/analysis.py to gammapy/analysis/core.py.

Otherwise, this looks good to me.

docs/analysis/index.rst Outdated Show resolved Hide resolved
@cdeil cdeil changed the title Rename scripts module into analysis Move Analysis to new gammapy.analysis Nov 11, 2019
@cdeil
cdeil approved these changes Nov 11, 2019
Copy link
Member

cdeil left a comment

@Bultako - Thanks!

@adonath - Please do a final review.

Copy link
Member

adonath left a comment

Thanks @Bultako, no further comments from my side.

@adonath adonath merged commit aaa8676 into gammapy:master Nov 12, 2019
10 of 11 checks passed
10 of 11 checks passed
Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
Scrutinizer Analysis: 48 updated code elements – Tests: passed
Details
codecov/patch 100% of diff hit (target 91.5%)
Details
codecov/project 91.5% (+<.01%) compared to 4379f04
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy Build #20191111.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.analysis automation moved this from In progress to Done Nov 12, 2019
@Bultako Bultako deleted the Bultako:hli branch Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants
You can’t perform that action at this time.