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

Fix SST-1M to be DC and not SC. Solves #905 #908

Merged
merged 1 commit into from
Dec 18, 2018
Merged

Fix SST-1M to be DC and not SC. Solves #905 #908

merged 1 commit into from
Dec 18, 2018

Conversation

thomasgas
Copy link

No description provided.

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.

What does DC stand for in this context?

And is this lookuptable really needed?
What does it give us, that's not readable from the simtel files?

@thomasgas
Copy link
Author

DC is Davies-Cotton. I think it's a useful piece of information (to distinguish Davies-Cotton from Schwarzschild-Couder). This look up table is fine for now but maybe these information can be also read from the mc. I would fix it for now and leave other changes to another PR.

@maxnoe
Copy link
Member

maxnoe commented Dec 18, 2018

But LST is not Davies Cotton afaik, its parabolic

@codecov
Copy link

codecov bot commented Dec 18, 2018

Codecov Report

Merging #908 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #908   +/-   ##
======================================
  Coverage    82.7%   82.7%           
======================================
  Files         186     186           
  Lines       10978   10978           
======================================
  Hits         9079    9079           
  Misses       1899    1899
Impacted Files Coverage Δ
ctapipe/instrument/optics.py 94.23% <ø> (ø) ⬆️

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 16457f0...cc89465. Read the comment docs.

@thomasgas
Copy link
Author

But LST is not Davies Cotton afaik, its parabolic

You're right. So i guess that by now "DC" is kind of an alias for "1-mirror-telescope" and "SC" is for "2-mirror-telescope". Which is clearly wrong...
what shall we do with this LUT? fill it with correct info or create it from the mc?

@kosack
Copy link
Contributor

kosack commented Dec 18, 2018

This info is there since one is not always reading an MC file, so you need a lookup for it. Also at the time, it wasn't possible to read it from the MCs - is there now some accessor?

For the names, I agree it's not ideal - we can easily change the optics type names, however, since they are not fixed or used anywhere yet. The main reason was to be able to adapt algorithms to different types, which could be simply the number of mirrors, but also may have other aspects such as off-axis corrections. I'm happy if we update these to be correct, or use better names.

  • SC: Swartzschild-Couder
  • DC: Davies Cotton
  • might need another for modified Davies-Cotton (e.g. LST) - is there a name for that mirror type? Parabolic DC?

@maxnoe
Copy link
Member

maxnoe commented Dec 18, 2018

might need another for modified Davies-Cotton

LST is afaik not a modified Davies–Cotton but a pure Segmented Parabolic Mirror

@thomasgas
Copy link
Author

so do you want to merge this or do you want to change LST optics.mirror_type in order to be consistent?

@maxnoe
Copy link
Member

maxnoe commented Dec 18, 2018

Apply this simple fix now, i'd say, rest later

@thomasgas thomasgas merged commit 25b35cf into cta-observatory:master Dec 18, 2018
@thomasgas thomasgas deleted the fix_sc_issue branch December 18, 2018 15:58
@thomasgas thomasgas mentioned this pull request Dec 18, 2018
@GernotMaier
Copy link
Contributor

For completeness: the 1-mirror MST has a 'modified DC design' (something between DC and parabolic).

I don't think this matters anywhere (unless one needs e.g. the smearing in time due to the optics or if one wants to fold the expected optical PSF into the image reconstruction), and knowing that it is a one or two-mirror instrument should be enough.

watsonjj added a commit to watsonjj/ctapipe that referenced this pull request Jan 7, 2019
* master: (22 commits)
  Remove all hillas_paramters but version 5, fixes cta-observatory#866  (cta-observatory#904)
  Fix docstring of EventSeeker, fixes cta-observatory#768 (cta-observatory#914)
  Do not set autodownload, fixes doc build (cta-observatory#910)
  Import bokeh.palletes correctly, fixes cta-observatory#911 (cta-observatory#912)
  Fix SST-1M to be DC and not SC. Solves cta-observatory#905 (cta-observatory#908)
  Fix a few test warnings (cta-observatory#902)
  Updates of NectarCam and LSTCam real data eventsource class (cta-observatory#812)
  Implemented FACT image cleaning (cta-observatory#862)
  remove `config=None, tool=None` in some tests (cta-observatory#897)
  Remove flow submodule (got moved to its own package) (cta-observatory#893)
  Cleaning the ctapipe folder. this has been empty for 3 years. (cta-observatory#892)
  Additional metadata from pyhessio, similar to cta-observatory#655 (cta-observatory#895)
  add scikit-learn to install_requires (cta-observatory#877)
  Add .mailmap (cta-observatory#894)
  Fix subarray peek. No more warnings. (cta-observatory#841)
  SimTelEventSource using pyeventio (cta-observatory#864)
  Use sparse neighbor matrix (cta-observatory#826)
  Add test how it should be (cta-observatory#842)
  fix errordef > 0. (cta-observatory#839)
  Fix warning about already closed hessio file (cta-observatory#834)
  ...

# Conflicts:
#	ctapipe/analysis/camera/chargeresolution.py
#	ctapipe/tools/extract_charge_resolution.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants