include stage1_config.json in package #1752

nbiederbeck · 2021-06-25T11:05:25Z

I included the stage1_config.json in ctapipe/examples, so the tests can use the packaged one. However, I am not sure about the naming of the directory.

This will fix #1750

maxnoe

This is great, thanks.

Two points:

a) I would probably just add the json file into a resources directory in ctapipe/tools/tests/resources. It is in this case not really an example, but a non-code file needed for the tests.
b) wee need to add importlib_resources to the required packages for python < 3.9.

nbiederbeck · 2021-06-25T11:22:43Z

a) I would probably just add the json file into a resources directory in ctapipe/tools/tests/resources. It is in this case not really an example, but a non-code file needed for the tests.

Yes, that is what I meant. Not really an example in this case anymore. But should we keep this also in the top-level examples? So that it is twice in the repo, but once in the package?

b) wee need to add importlib_resources to the required packages for python < 3.9.

Is it possible to require this only if python < 3.9 ? I just put it into tests_require for now

codecov · 2021-06-25T11:26:48Z

Codecov Report

Merging #1752 (1ab8661) into master (cdd2fe5) will decrease coverage by 0.25%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1752      +/-   ##
==========================================
- Coverage   92.04%   91.78%   -0.26%     
==========================================
  Files         187      187              
  Lines       14696    14706      +10     
==========================================
- Hits        13527    13498      -29     
- Misses       1169     1208      +39

Impacted Files	Coverage Δ
ctapipe/tools/tests/test_merge.py	`100.00% <100.00%> (ø)`
ctapipe/tools/tests/test_process.py	`95.29% <100.00%> (+0.17%)`	⬆️
ctapipe/tools/fileinfo.py	`18.36% <0.00%> (-71.43%)`	⬇️
ctapipe/tools/utils.py	`80.00% <0.00%> (-13.34%)`	⬇️
ctapipe/conftest.py	`95.41% <0.00%> (ø)`
ctapipe/tools/process.py	`95.45% <0.00%> (ø)`
ctapipe/image/image_processor.py	`100.00% <0.00%> (ø)`
ctapipe/image/tests/test_image_processor.py	`100.00% <0.00%> (ø)`
ctapipe/reco/tests/test_shower_processor.py	`100.00% <0.00%> (ø)`
ctapipe/io/eventsource.py	`94.00% <0.00%> (+0.25%)`	⬆️

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 cdd2fe5...1ab8661. Read the comment docs.

nbiederbeck · 2021-06-25T11:30:26Z

Is it possible to require this only if python < 3.9 ? I just put it into tests_require for now

for future reference: yes you can, with environment markers from pep 508

setup.py

ctapipe/tools/tests/test_tools.py

maxnoe

I needed to add "resouces/*" to package data to make setuptools include a json file in ctapipe/tools/tests/resources/ to the wheels / source dists:

 81 ⋮ 81 │    ],
 82 ⋮ 82 │    zip_safe=False,
 83 ⋮ 83 │    entry_points=entry_points,
 84 ⋮    │    package_data={"": ["tools/bokeh/*.yaml", "tools/bokeh/templates/*.html"]},
    ⋮ 84 │    package_data={
    ⋮ 85 │        "": [
    ⋮ 86 │            "tools/bokeh/*.yaml",
    ⋮ 87 │            "tools/bokeh/templates/*.html",
    ⋮ 88 │            "resources/*",
    ⋮ 89 │        ]
    ⋮ 90 │    },
 85 ⋮ 91 │)

nbiederbeck · 2021-06-25T13:22:22Z

I can reproduce that issue, after cleaning up (rm -rf build *.egg-info)

weird, that the tests still pass ..

maxnoe · 2021-06-28T09:30:02Z

Maybe it would be worth to use this setup in the CI to make sure it works.

So I'd suggest to remove the -e in the pip install and use pytest --pyargs ctapipe to run the tests.

kosack · 2021-06-28T15:41:34Z

It may be worth waiting for #1726 since that introduces several more "standard" config files that need to be used for testing. I only haven't had time to write the tests.

Of course, we eventually need to create a configuration repo in the CTA-GitLab to keep track of real (non-testing) configurations.

Which reminds me, it may be nice to allow configurations from a git repo rather than only as local files, using the same URL scheme that we use for other files. That way we can specify a path like --config git:// with the filename and version tag. See #1754

maxnoe · 2021-06-28T15:46:46Z

Wouldn't it make sense to do it the other way around? Merge this here first and then you can just add those new config files to the ctapipe/tools/tests/resources folder in the other PR?

kosack · 2021-06-28T15:52:47Z

Wouldn't it make sense to do it the other way around? Merge this here first and then you can just add those new config files to the ctapipe/tools/tests/resources folder in the other PR?

Yes that's also ok. It has just been hard to maintain that branch with all the changes

review-notebook-app · 2021-08-03T13:48:50Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

nbiederbeck · 2021-08-03T13:51:38Z

ouch, it seems like I need to review how to do git

nbiederbeck · 2021-08-03T14:49:01Z

sorry if anyone was notified unnecessarily.

I now added the resources for the ProcessorTool and resolved the conflicts that were shown

kosack · 2021-08-03T15:07:26Z

The only issue I have here is that these configs really are examples, that the user may want to take and modify, not just used for testing. They are supposed to define the reference analysis. So perhaps we should keep two copies of them? One for tests, and one for examples? Otherwise they are now hard to find.

Or perhaps better yet: we could make a Tool that sets up a directory for user analysis (ctapipe-quickstart or something), that writes a copy of the standard configs to a user-specified directory with a README. It could even fetch some example files. Essentially, it would set up a working directory with templates for the configs. For now, it could be simple, but later it could have some more options. There is also ProcessorTool().generate_config_file() (a feature from trailets.config that generates a commented config file, but only in Python format).

To start it could be really simple: it has no options other than --directory which defaults to ".". A user would just run:

% ls

% ctapipe-quickstart 
% ls 
stage1_config.json  stage2_config.json  training_config.json

And it would internally get the files using the package resources and writes them to the directory.

kosack · 2021-08-03T15:24:55Z

By the way, could we not just put these configs on the datatserver in the latest version of ctapipe-extra like we do with all other testing files? Thatt would make this PR unneeded, since you could just use get_dataset_path("stage1_config.json") ? Then we could still use a quickstart tool to download them from the command-line, but the test would work automatically

maxnoe · 2021-08-03T15:58:05Z

By the way, could we not just put these configs on the datatserver in the latest version of ctapipe-extra like we do with all other testing files?

I guess that these files will need updates more often than data files. Which makes putting them on the data server really suboptimal, since only very limited number of people have access to them and somebody preparing a PR that needs configs to be adjusted cannot do it while developing. Also, these are small text files, so are well suited for git.

kosack

Ok let's merge this then, and I'll open a PR for a tool to extract them for users,.

nbiederbeck changed the title ~~Importlib resources~~ include stage1_config.json in package Jun 25, 2021

maxnoe requested changes Jun 25, 2021

View reviewed changes

maxnoe reviewed Jun 25, 2021

View reviewed changes

setup.py Outdated Show resolved Hide resolved

maxnoe reviewed Jun 25, 2021

View reviewed changes

ctapipe/tools/tests/test_tools.py Outdated Show resolved Hide resolved

maxnoe requested changes Jun 25, 2021

View reviewed changes

kosack previously approved these changes Jun 28, 2021

View reviewed changes

maxnoe previously approved these changes Jun 28, 2021

View reviewed changes

nbiederbeck dismissed stale reviews from maxnoe and kosack via 59740bc June 28, 2021 10:21

maxnoe mentioned this pull request Jun 28, 2021

Change REF_WIDTH to REFWIDTH to fit into 8 characters for FITS, fixes #1452 #1753

Merged

kosack previously approved these changes Jul 22, 2021

View reviewed changes

nbiederbeck dismissed kosack’s stale review via c0f480d August 3, 2021 13:48

nbiederbeck requested a review from HealthyPear as a code owner August 3, 2021 13:48

nbiederbeck added 7 commits August 3, 2021 16:14

Include stage1_config.json in package.

ccf56d0

Use included stage1_config.json.

55d0dc8

Add importlib_resources to tests_require.

32d6b10

Move stage1_config.json to tool test resources.

b80f978

Require importlib_resources only for python < 3.9

b05aaa6

Remove examples/*.json from package_data

ad1111f

Add the resources to package_data.

74b060a

Test the inclusion of resources in CI.

d88c8e7

nbiederbeck force-pushed the importlib-resources branch from c0f480d to d88c8e7 Compare August 3, 2021 14:20

nbiederbeck changed the title ~~include stage1_config.json in package~~ Draft: include stage1_config.json in package Aug 3, 2021

nbiederbeck marked this pull request as draft August 3, 2021 14:21

nbiederbeck removed the request for review from HealthyPear August 3, 2021 14:21

Use importlib resources with ProcessorTool

1ab8661

nbiederbeck marked this pull request as ready for review August 3, 2021 14:47

nbiederbeck changed the title ~~Draft: include stage1_config.json in package~~ include stage1_config.json in package Aug 3, 2021

nbiederbeck requested review from maxnoe and kosack August 3, 2021 14:49

maxnoe approved these changes Aug 3, 2021

View reviewed changes

kosack approved these changes Aug 3, 2021

View reviewed changes

kosack merged commit 54fa679 into cta-observatory:master Aug 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

include stage1_config.json in package #1752

include stage1_config.json in package #1752

nbiederbeck commented Jun 25, 2021

maxnoe left a comment

nbiederbeck commented Jun 25, 2021 •

edited

Loading

codecov bot commented Jun 25, 2021 •

edited

Loading

nbiederbeck commented Jun 25, 2021

maxnoe left a comment

nbiederbeck commented Jun 25, 2021

maxnoe commented Jun 28, 2021

kosack commented Jun 28, 2021 •

edited

Loading

maxnoe commented Jun 28, 2021

kosack commented Jun 28, 2021

review-notebook-app bot commented Aug 3, 2021

nbiederbeck commented Aug 3, 2021

nbiederbeck commented Aug 3, 2021

kosack commented Aug 3, 2021 •

edited

Loading

kosack commented Aug 3, 2021 •

edited

Loading

maxnoe commented Aug 3, 2021

kosack left a comment

include stage1_config.json in package #1752

include stage1_config.json in package #1752

Conversation

nbiederbeck commented Jun 25, 2021

maxnoe left a comment

Choose a reason for hiding this comment

nbiederbeck commented Jun 25, 2021 • edited Loading

codecov bot commented Jun 25, 2021 • edited Loading

Codecov Report

nbiederbeck commented Jun 25, 2021

maxnoe left a comment

Choose a reason for hiding this comment

nbiederbeck commented Jun 25, 2021

maxnoe commented Jun 28, 2021

kosack commented Jun 28, 2021 • edited Loading

maxnoe commented Jun 28, 2021

kosack commented Jun 28, 2021

review-notebook-app bot commented Aug 3, 2021

nbiederbeck commented Aug 3, 2021

nbiederbeck commented Aug 3, 2021

kosack commented Aug 3, 2021 • edited Loading

kosack commented Aug 3, 2021 • edited Loading

maxnoe commented Aug 3, 2021

kosack left a comment

Choose a reason for hiding this comment

nbiederbeck commented Jun 25, 2021 •

edited

Loading

codecov bot commented Jun 25, 2021 •

edited

Loading

kosack commented Jun 28, 2021 •

edited

Loading

kosack commented Aug 3, 2021 •

edited

Loading

kosack commented Aug 3, 2021 •

edited

Loading