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

dump CameraReadout files in ctapipe-dump-instrument #1451

Merged
merged 4 commits into from
Nov 13, 2020

Conversation

kosack
Copy link
Contributor

@kosack kosack commented Oct 9, 2020

Now that CameraDefinitions need both a camgeom and camreadout file, lets generate them

Needed to generate sample .camreadout.fits.gz files for testing purposes in ctapipe-extra

This PR also removes the unused table_type, table_name options from DumpInstrumentTool._get_file_format_info()

needed to generate sample .camreadout.fits.gz files for testing purposes in ctapipe-extra
maxnoe
maxnoe previously approved these changes Oct 9, 2020
Copy link
Member

@maxnoe maxnoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these hierarchical descriptions not relevant anymore?

@kosack
Copy link
Contributor Author

kosack commented Oct 12, 2020

Are these hierarchical descriptions not relevant anymore

What do you mean by hierarchical description?

@maxnoe
Copy link
Member

maxnoe commented Oct 14, 2020

dl0.tel.svc.camera

@kosack
Copy link
Contributor Author

kosack commented Oct 14, 2020

Ah, those are from an old version of the data model, so in any case need to change. However, I'm not sure its the best thing to use for the "role" of the file in the provenance. So far we have no rule for how to name roles, but I guess it's better to say why the data was used, not what kind of data it is (e.g. that the file was opened to construct a "CameraGeometry" object).

Or we change it to reflect the new data model better, in which case it should be Configuration/Instrument/Telescope/Camera/Geometry or something like that. Perhaps this should be a separate issue: how to name roles for files opened in ctapipe, and we can update that elsewhere (for now I just want to remove the outdated data model names).

@maxnoe
Copy link
Member

maxnoe commented Oct 19, 2020

@kosack the build failed due to some file not found error

@kosack
Copy link
Contributor Author

kosack commented Oct 19, 2020

Ah, yes, the problem is that we do not have an up-to-date optics description in ctapipe-extra. I will update it, then this should work (the current one uses the old telescope naming scheme, and is named differently).

vuillaut
vuillaut previously approved these changes Oct 21, 2020
Copy link
Member

@vuillaut vuillaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build is still not passing though.

@kosack kosack dismissed stale reviews from vuillaut and maxnoe via 5e7a5cb October 28, 2020 16:38
@maxnoe
Copy link
Member

maxnoe commented Nov 9, 2020

@kosack the test fails because you used --infile instead of --input

@maxnoe
Copy link
Member

maxnoe commented Nov 9, 2020

Also, you should add the cwd=tmpdir

@codecov
Copy link

codecov bot commented Nov 9, 2020

Codecov Report

Merging #1451 (fe07e4f) into master (cb1dace) will increase coverage by 0.05%.
The diff coverage is 90.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1451      +/-   ##
==========================================
+ Coverage   90.78%   90.83%   +0.05%     
==========================================
  Files         188      192       +4     
  Lines       13562    13685     +123     
==========================================
+ Hits        12312    12431     +119     
- Misses       1250     1254       +4     
Impacted Files Coverage Δ
ctapipe/io/tests/test_dl1eventsource.py 95.83% <66.66%> (ø)
ctapipe/io/dl1writer.py 85.14% <85.14%> (ø)
ctapipe/tools/dump_instrument.py 87.95% <95.00%> (+0.93%) ⬆️
ctapipe/image/image_processor.py 95.65% <95.65%> (ø)
ctapipe/containers.py 100.00% <100.00%> (ø)
ctapipe/core/provenance.py 91.60% <100.00%> (+0.19%) ⬆️
ctapipe/image/__init__.py 100.00% <100.00%> (ø)
ctapipe/image/concentration.py 100.00% <100.00%> (ø)
ctapipe/image/leakage.py 100.00% <100.00%> (ø)
ctapipe/image/tests/test_concentration.py 95.23% <100.00%> (ø)
... and 12 more

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 cb0cb37...fe07e4f. Read the comment docs.

@maxnoe maxnoe requested a review from vuillaut November 9, 2020 15:10
@maxnoe maxnoe added this to the v0.10.0 milestone Nov 9, 2020
@maxnoe
Copy link
Member

maxnoe commented Nov 9, 2020

After merging, you produce the readout files for the cameras we have in ctapipe extra and add them to ctapipe extra?

@kosack
Copy link
Contributor Author

kosack commented Nov 9, 2020

After merging, you produce the readout files for the cameras we have in ctapipe extra and add them to ctapipe extra?

Yes, that's the idea. Then we can release another version of ctapipe-extra (or we add them to the server, and merge #1498 )

@maxnoe
Copy link
Member

maxnoe commented Nov 9, 2020

Or both, for the moment it is probably the easiest to add them to ctapipe-extra so johan can just download the newest version.

@maxnoe maxnoe merged commit 9895fc6 into cta-observatory:master Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants