Fixes for Syntax Errors and other broken stuff #391

maxnoe · 2020-05-01T10:30:03Z

Several things in the current master are broken

I added tests for all scripts to see if at least running the script with --help works and
fixed a lot of simply broken stuff (Syntax errors due to missing or additional commas, missing imports)

ctaplot as dependency was also missing.

For convenience I also added ipython and jupyter to the environment as I just spend 15 Minutes of my life debugging why import ctapipe failed in the lst environment until I realized I was trying with /usr/bin/ipython

The eventio version was incompatible with the one required by ctapipe 0.7

codecov · 2020-05-01T10:44:31Z

Codecov Report

Merging #391 into master will decrease coverage by 1.82%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master     #391      +/-   ##
==========================================
- Coverage   45.83%   44.01%   -1.83%     
==========================================
  Files          68       71       +3     
  Lines        4963     5185     +222     
==========================================
+ Hits         2275     2282       +7     
- Misses       2688     2903     +215

Impacted Files	Coverage Δ
lstchain/scripts/lstchain_dl1_muon_analysis.py	`0.00% <ø> (ø)`
...n/scripts/onsite/onsite_create_calibration_file.py	`0.00% <0.00%> (ø)`
...scripts/onsite/onsite_create_drs4_pedestal_file.py	`0.00% <0.00%> (ø)`
lstchain/calib/camera/calibrator.py	`28.20% <40.00%> (ø)`
lstchain/calib/camera/__init__.py	`100.00% <100.00%> (ø)`
lstchain/calib/camera/tests/test_pedestals.py	`100.00% <100.00%> (ø)`
lstchain/scripts/tests/test_lstchain_scripts.py	`98.41% <100.00%> (+0.13%)`	⬆️

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 d2f11f8...14d2591. Read the comment docs.

rlopezcoto · 2020-05-01T14:59:29Z

Regarding including ctaplot as a dependency, we have discussed about it starting here:
#131 (comment)

and then here:
#279

the whole point was:

Not to include as a dependency a personal package: @vuillaut said that they were planning to move it to cta-observatory
Get a more info about the I/O of the plotting to be easily used (cleanup and bugfix in viz dl2 #279 (comment))

lstchain/calib/camera/__init__.py

lstchain/scripts/lstchain_data_create_drs4_pedestal_file.py

lstchain/scripts/onsite/onsite_create_calibration_file.py

rlopezcoto · 2020-05-01T15:05:14Z

setup.py

@@ -42,7 +42,8 @@ def find_scripts(script_dir, prefix):
    install_requires=[
        'astropy',
        'ctapipe~=0.7.0',
-        'eventio~=1.0',
+        'ctaplot',


already discussed in the comments

setup.py

maxnoe · 2020-05-01T16:03:40Z

Regarding including ctaplot as a dependency, we have discussed about it starting here:

But it is a dependency. It's imported in a cli program and used.

Either, you don't want it as a dependency, than it needs to be removed completely or you use it, than it needs to be included.

maxnoe · 2020-05-02T09:21:41Z

Not to include as a dependency a personal package: @vuillaut said that they were planning to move it to cta-observatory

It seems this has happend: https://github.com/cta-observatory/ctaplot

morcuended

change --input_file to --input-file and --output_file to --output-file

maxnoe · 2020-05-02T11:31:56Z

change --input_file to --input-file and --output_file to --output-file

I would say this has nothing to do with this PR. These are small fixes to obviously broken things.

No API changes are made here.

morcuended · 2020-05-02T11:50:51Z

change --input_file to --input-file and --output_file to --output-file

I would say this has nothing to do with this PR. These are small fixes to obviously broken things.

No API changes are made here.

Right, but I was trying to produce new drs4 files with the onsite script and I found it's not working (apart from the os import) because the naming of command line options has not been updated. Since this PR is fixing broken stuff I thought it could fit here as well. I can open a new PR if not.

rlopezcoto · 2020-05-02T12:00:22Z

It seems this has happend: https://github.com/cta-observatory/ctaplot

great, what about the other point @vuillaut?

rlopezcoto · 2020-05-02T13:45:18Z

Right, but I was trying to produce new drs4 files with the onsite script and I found it's not working (apart from the os import) because the naming of command line options has not been updated. Since this PR is fixing broken stuff I thought it could fit here as well. I can open a new PR if not.

I only modified the scripts within scripts/, did not want to touch those under scripts/onsite/ not to mess things up more for LSTOSA, but @morcuended it would be great if you can adapt them to the same naming as in #380

maxnoe · 2020-05-02T13:56:49Z

great, what about the other point @vuillaut?

This point doesn't really matter to this PR. RIght now, in the master branch, ctaplot is a dependency.
Thus it goes into setup.py.

If you want to get rid of this depedency, do it. But as long as it is used, it belongs in setup.py.

rlopezcoto · 2020-05-02T14:04:33Z

There are at the moment two scripts where ctaplot is used:
lstchain_mc_rfperformance
lstchain_mc_sensitivity

It is included as an option in the first one:

try:
    import ctaplot
except ImportError as e:
    print("ctaplot not installed, some plotting function will be missing")

could you include it as an option on the second one instead of including it in the setup until this is worked out?

This change does not belong here

morcuended · 2020-05-03T11:29:26Z

I updated the branch with master latest changes (don't know if I should have done it) and now the tests are failing because of onsite scripts. Otherwise, it seems fine to me.

maxnoe · 2020-05-03T11:32:36Z

@morcuended sure, no problem

maxnoe · 2020-05-03T11:33:25Z

@morcuended what's the matter with the onsite scripts?

Ah, found it. there is the __init__.py missing, and that's why it's not included by find_packages

lstchain/scripts/lstchain_data_create_time_calibration_file.py

lstchain/scripts/onsite/onsite_create_calibration_file.py

maxnoe added 7 commits May 1, 2020 12:09

Require correct version of eventio for ctapipe 0.7

0e1a6d0

Add ipython and jupyter to environment for convenience

76ae62a

Remove setting mpl backend, use env variable MPLBACKEND instead

e7a161b

Fix syntax errors in scripts

8a1dc5e

Add ctapplot

f1da53e

Test all scripts if at least --help works

9bc7443

Fix two more scripts

f701768

Remove star imports, raise if calib file could not be read

88cd115

rlopezcoto reviewed May 1, 2020

View reviewed changes

Fix scikit learn package name in setup.py

231f680

morcuended previously requested changes May 2, 2020

View reviewed changes

rlopezcoto mentioned this pull request May 2, 2020

Fix dl1 file naming to get rid of the stream number and "fits" extension #344

Merged

Merge remote-tracking branch 'origin/master' into fixes

c8aebdc

maxnoe force-pushed the fixes branch from 738c5c4 to c8aebdc Compare May 2, 2020 14:02

maxnoe requested review from rlopezcoto and morcuended May 2, 2020 14:03

maxnoe added 2 commits May 2, 2020 16:37

Find entry points automatically

33518a0

Update test_lstchain_scripts.py

1608e7c

Merge branch 'master' into fixes

6a67a2e

Add missing __init__.py for onsite scripts

6815b3f

morcuended previously approved these changes May 3, 2020

View reviewed changes

morcuended reviewed May 3, 2020

View reviewed changes

lstchain/scripts/lstchain_data_create_time_calibration_file.py Outdated Show resolved Hide resolved

rlopezcoto mentioned this pull request May 4, 2020

cleanup and bugfix in viz dl2 #279

Merged

Merge branch 'master' into fixes

debcb5d

morcuended dismissed their stale review via debcb5d May 4, 2020 16:50

morcuended reviewed May 4, 2020

View reviewed changes

lstchain/scripts/onsite/onsite_create_calibration_file.py Outdated Show resolved Hide resolved

Remove duplicated import

102c2d8

rlopezcoto approved these changes May 5, 2020

View reviewed changes

Merge branch 'master' into fixes

14d2591

morcuended approved these changes May 5, 2020

View reviewed changes

maxnoe merged commit fd88b77 into master May 5, 2020

maxnoe deleted the fixes branch May 5, 2020 07:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for Syntax Errors and other broken stuff #391

Fixes for Syntax Errors and other broken stuff #391

maxnoe commented May 1, 2020 •

edited

Loading

codecov bot commented May 1, 2020 •

edited

Loading

rlopezcoto commented May 1, 2020

rlopezcoto May 1, 2020

maxnoe commented May 1, 2020 •

edited

Loading

maxnoe commented May 2, 2020

morcuended left a comment

maxnoe commented May 2, 2020 •

edited

Loading

morcuended commented May 2, 2020

rlopezcoto commented May 2, 2020

rlopezcoto commented May 2, 2020

maxnoe commented May 2, 2020

rlopezcoto commented May 2, 2020

morcuended commented May 3, 2020

maxnoe commented May 3, 2020

maxnoe commented May 3, 2020 •

edited

Loading

Fixes for Syntax Errors and other broken stuff #391

Fixes for Syntax Errors and other broken stuff #391

Conversation

maxnoe commented May 1, 2020 • edited Loading

codecov bot commented May 1, 2020 • edited Loading

Codecov Report

rlopezcoto commented May 1, 2020

rlopezcoto May 1, 2020

Choose a reason for hiding this comment

maxnoe commented May 1, 2020 • edited Loading

maxnoe commented May 2, 2020

morcuended left a comment

Choose a reason for hiding this comment

maxnoe commented May 2, 2020 • edited Loading

morcuended commented May 2, 2020

rlopezcoto commented May 2, 2020

rlopezcoto commented May 2, 2020

maxnoe commented May 2, 2020

rlopezcoto commented May 2, 2020

morcuended commented May 3, 2020

maxnoe commented May 3, 2020

maxnoe commented May 3, 2020 • edited Loading

maxnoe commented May 1, 2020 •

edited

Loading

codecov bot commented May 1, 2020 •

edited

Loading

maxnoe commented May 1, 2020 •

edited

Loading

maxnoe commented May 2, 2020 •

edited

Loading

maxnoe commented May 3, 2020 •

edited

Loading