Feature/post dl2 analysis #509

mexanick · 2020-09-07T08:54:23Z

Draft implementation of the Fast Analysis LST Plots (fast alps) analysis framework.
It aims to provide a common framework for DL2-level analysis implementation and simplify the integration of other analyses by:

Provide common data mangling utilities
Provide common infrastructure for documentation (TBA)
Provide common infrastructure for correctness and performance tests (TBA)

review-notebook-app · 2020-09-07T08:54:27Z

Check out this pull request on

Review Jupyter notebook visual diffs & provide feedback on notebooks.

Powered by ReviewNB

vuillaut

Hi.

Thank you for your contribution, this will be useful!
Before we can make a proper review, please address the unit tests issues and the two bugs I noted here (the comment on the data_path is more of an improvement feature).

lstchain/analysis/fast_alps.py

docs/examples/fast_alps/config_on_off.toml

lstchain/io/io.py

vuillaut · 2020-09-07T15:22:26Z

Also, as this is completely part of lstchain, I would rename the submodule as analysis.dl2 or something similar. fast_alps would not make much sense to a newcomer.

codecov · 2020-09-28T13:31:46Z

Codecov Report

Merging #509 (bc8a4a4) into master (6800e27) will decrease coverage by 1.02%.
The diff coverage is 5.35%.

@@            Coverage Diff             @@
##           master     #509      +/-   ##
==========================================
- Coverage   38.54%   37.52%   -1.03%     
==========================================
  Files          80       82       +2     
  Lines        7116     7339     +223     
==========================================
+ Hits         2743     2754      +11     
- Misses       4373     4585     +212

Impacted Files	Coverage Δ
lstchain/analysis/post_dl2.py	`0.00% <0.00%> (ø)`
lstchain/scripts/lstchain_post_dl2.py	`0.00% <0.00%> (ø)`
lstchain/visualization/plot_dl2.py	`49.14% <8.33%> (-5.72%)`	⬇️
lstchain/reco/utils.py	`66.08% <18.18%> (-7.08%)`	⬇️
lstchain/io/io.py	`77.19% <22.72%> (-3.24%)`	⬇️

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 6800e27...bc8a4a4. Read the comment docs.

mexanick · 2020-10-12T16:21:24Z

Looks like the CI is stalled for some reason. Does anyone know what could be the reason?

morcuended · 2020-10-12T16:46:54Z

Looks like the CI is stalled for some reason. Does anyone know what could be the reason?

Could it be the conflicting files?
lstchain/io/io.py
lstchain/reco/utils.py

rlopezcoto · 2020-10-12T21:16:10Z

@mexanick if the PR is ready, you should mark the "Ready for review" button. In any case and as @morcuended pointed out, you need to solve the conflicts with those two files.

- Data mangling utilities added to lstchain.io module - Camera-related utilities are placed under lstchain.reco.utils module - launching script and anlaysis routines are added too - TODO: reshuffle plotting utilities and offload statistics to gammapy

- Fix memory-efficient data reading and subrun merging - Provide configuration examples - Minor bugfixes in plotting

… different nights

mexanick · 2020-10-13T07:52:45Z

I'm not sure if that was because of the conflict (honestly, I doubt it, looks like some travis glitch), but I have resolved it.

maxnoe · 2020-10-13T08:00:58Z

@mexanick Travis cannot run the PR build of there are conflicts. The PR build (unlike the "branch" build) is done by merging the PR branch into master and then running the tests.

This is not possible when there are conflicts.

https://docs.travis-ci.com/user/pull-requests/#my-pull-request-isnt-being-built

mexanick · 2020-10-13T08:33:47Z

@mexanick Travis cannot run the PR build of there are conflicts. The PR build (unlike the "branch" build) is done by merging the PR branch into master and then running the tests.

This is not possible when there are conflicts.

https://docs.travis-ci.com/user/pull-requests/#my-pull-request-isnt-being-built

Thanks! I'm used more to gitlab CI which would just build the branch first, so it goes through anyway.

vuillaut · 2020-10-24T08:18:35Z

@mexanick once the tests are passing I can do the review

mexanick · 2020-10-24T08:36:53Z

@mexanick once the tests are passing I can do the review

Actually, the failed test is radec to camera function which failed being unable to resolve the url, I'm not sure how I can fix it... I'm afraid that's a systematic issue which affects all the on-going PRs. And considering the Codacy review, it is about unused variable fig in fig, axes = plt.subplot(...)
Please, advise on how to proceed

Implementing some comments

- Update documentation - Passing the telescope focal length as a parameter (can be retrieved from data, but this is not implemented. Can be done in user scripts if needed) - Conversion factor for theta2 calculation is left as is (parameter). Can also be sourced from DL2 file but requires reading in the complete geometry to get the distance between pixel centers and focal length - Update the statistics to conform with latest gammapy release - Provide the interface to store the resulting images through configuration file

- Update jupyter notebook to represent interactive and non-interactive modes - Fix theta2 computation w.r.t. dimensional data - Minor logging and plotting updates

vuillaut · 2020-12-23T17:41:17Z

Hi @mexanick
If you can comment on the requests I had, I think we can work this PR out to finalize it.

mexanick · 2021-01-05T12:59:00Z

hi @vuillaut
Most of your comments are implemented except conversion factor in theta2 calculation (it is possible to do, but will require a certain overhead as mentioned in the corresponding commit message).

vuillaut · 2021-01-15T14:35:05Z

I still think that some things could be improved but it works and would be useful to have as long as we don't have a proper DL3 pipeline so I will approve

vuillaut requested changes Sep 7, 2020

View reviewed changes

lstchain/analysis/fast_alps.py Outdated Show resolved Hide resolved

docs/examples/fast_alps/config_on_off.toml Outdated Show resolved Hide resolved

lstchain/io/io.py Outdated Show resolved Hide resolved

rlopezcoto marked this pull request as draft September 14, 2020 09:06

mexanick requested a review from vuillaut October 1, 2020 11:29

mexanick changed the title ~~[WIP] Feature/post dl2 analysis~~ Feature/post dl2 analysis Oct 12, 2020

mexanick added 11 commits October 13, 2020 07:49

Using the gammapy.stat instead of homemade statistical calculation

b184f1d

Add fast_alps notebook

135331d

Resolve remaining issues with the fast_alps integration

37777a8

- Fix memory-efficient data reading and subrun merging - Provide configuration examples - Minor bugfixes in plotting

Fix remaning issues with the analyses

019b817

Fix unused imports and trailing whitespaces

e9532b2

Fix remaining codacy issues

f45b0c4

Fix naming of the module, add init file

f338cb5

Improve observation time computation

c0bdb08

Finish module names migration, implement ability to analyse runs from…

d41d38f

… different nights

Update the notebook

e910a2b

mexanick force-pushed the feature/post-DL2-analysis branch from f039e1f to e910a2b Compare October 13, 2020 07:51

mexanick marked this pull request as ready for review October 13, 2020 07:53

Remove extra pandas import

f35fa1a

Merge branch 'master' into feature/post-DL2-analysis

be6dbdb

mexanick and others added 13 commits November 2, 2020 14:31

Update documentaion and variable/method names

0e9203e

Merge remote-tracking branch 'upstream/feature/post-DL2-analysis'

4e9da2b

removed comments

92d91a4

rename function read_dl2_sub_run to read_dl2_params

b42b628

update docstring plot_wobble function

9db239d

update docstring plot_1d_excess function

eb6f265

update variable for cycle in the plot_wobble function

af2d290

updated read_dl2_params function name

6aeb5d6

fix bug on WStatCountsStatistic excess and significance attributes

265e21b

improved log output and for cycle variable name

a6f7c74

improved docstring

caf02bd

revert to original logger output

033422a

add reference to API change for significance computation with gammapy

f25acc0

mexanick closed this Dec 9, 2020

mexanick force-pushed the feature/post-DL2-analysis branch from 0e9203e to 745c824 Compare December 9, 2020 12:11

mexanick and others added 3 commits December 9, 2020 13:49

Merge branch 'feature/post-DL2-analysis' into master

c6af603

Merge pull request #1 from lfoffano/master

035f27f

Implementing some comments

mexanick reopened this Dec 10, 2020

Update of notebook and logging, fix remaining bugs

b38e2b6

- Update jupyter notebook to represent interactive and non-interactive modes - Fix theta2 computation w.r.t. dimensional data - Minor logging and plotting updates

mexanick requested a review from vuillaut January 12, 2021 08:49

Merge branch 'master' into feature/post-DL2-analysis

f9feead

vuillaut approved these changes Jan 15, 2021

View reviewed changes

Merge branch 'master' into feature/post-DL2-analysis

bc8a4a4

vuillaut merged commit b8e3021 into cta-observatory:master Feb 3, 2021

rlopezcoto mentioned this pull request Feb 10, 2021

add missing toml from environment/setup #588

Merged

chaimain mentioned this pull request Feb 24, 2021

Tools to create DL3 files with IRFs generated using pyirf functions. #593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/post dl2 analysis #509

Feature/post dl2 analysis #509

mexanick commented Sep 7, 2020

review-notebook-app bot commented Sep 7, 2020

vuillaut left a comment

vuillaut commented Sep 7, 2020

codecov bot commented Sep 28, 2020 •

edited

Loading

mexanick commented Oct 12, 2020

morcuended commented Oct 12, 2020

rlopezcoto commented Oct 12, 2020

mexanick commented Oct 13, 2020

maxnoe commented Oct 13, 2020

mexanick commented Oct 13, 2020

vuillaut commented Oct 24, 2020

mexanick commented Oct 24, 2020

vuillaut commented Dec 23, 2020

mexanick commented Jan 5, 2021

vuillaut commented Jan 15, 2021

Feature/post dl2 analysis #509

Feature/post dl2 analysis #509

Conversation

mexanick commented Sep 7, 2020

review-notebook-app bot commented Sep 7, 2020

vuillaut left a comment

Choose a reason for hiding this comment

vuillaut commented Sep 7, 2020

codecov bot commented Sep 28, 2020 • edited Loading

Codecov Report

mexanick commented Oct 12, 2020

morcuended commented Oct 12, 2020

rlopezcoto commented Oct 12, 2020

mexanick commented Oct 13, 2020

maxnoe commented Oct 13, 2020

mexanick commented Oct 13, 2020

vuillaut commented Oct 24, 2020

mexanick commented Oct 24, 2020

vuillaut commented Dec 23, 2020

mexanick commented Jan 5, 2021

vuillaut commented Jan 15, 2021

codecov bot commented Sep 28, 2020 •

edited

Loading