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

Flat-field camera corrections #870

Closed
wants to merge 115 commits into from

Conversation

FrancaCassol
Copy link
Contributor

New code to calculate flat-field coefficients from flasher calibration events.

For that, new containers are defined in container.py

  • MonitorDataContainer : general monitor data container
  • FlatFieldContainer and FlatFieldCameraContainer: containers for the flat-field calculated coefficients

The code is based on a FlatFieldFactory (place in calib/camera) which permits to select a specific FlatFieldCalculator.

For the moment one calculator is implemented:

  • FlasherFlatFieldCalculator : based on the best algorithm described by S. Fegan in MST-CAM-TN-0060

Also, a tool is provided in ctapipe/examples:

  • calc_flatfield.py : which selects the waveform cleaner, the charge integrator and the desired FlatFieldCalculator. It performs a loop on the events and writes the FlatFieldCameraContainer in a hdf5 file.

NB: this PR will work only when PR #812 will be accepted (help on that side is very welcome)

FrancaCassol and others added 30 commits September 28, 2017 12:47
…STCameraContainer (Fields: LSTEventContainer and LSTServiceContainer)
…le with version 1.0.2 of the protozfit library
…_sub caliculation)

Patched setting of event trigger time for MC events FlatFieldCalculator.calculate_relative_gain
@FrancaCassol
Copy link
Contributor Author

Thanks Dominik, I wish you to find easily the problem.

I see that you made a merge in my repository, can I make a pull it to my local repository and then a push again? (I have some corrections to push to the PR coming from the MC test)

@dneise
Copy link
Member

dneise commented Jan 17, 2019

can I make a pull it to my local repository and then a push again

yes, sure.

@dneise
Copy link
Member

dneise commented Jan 17, 2019

I think we need #925 before we can expect the tests to work again

@dneise
Copy link
Member

dneise commented Jan 18, 2019

Very good @FrancaCassol .. I think we are close to have the master fixed again .. so that this PR can finally also be merged.

I cancel the tests for the time being, since they will still fail due to the problems with the master. .. might take another hour.

@FrancaCassol
Copy link
Contributor Author

Dominik, I am going to move the test_pedestal.

Added a a window_shift argument to BaselineWaveformCleaner
@dneise
Copy link
Member

dneise commented Jan 18, 2019

Okay do whatever you like ... just for the moment do not (yet) expect the tests to pass

@FrancaCassol
Copy link
Contributor Author

Hi,
I have now a "strategy" question: I have a new version of the LST reader (necessary for a full interpretation of the data, e.g. from TIB) that I would like to pull, can I merge it to this PR?

@dneise
Copy link
Member

dneise commented Jan 22, 2019

Ah Hello Franca, regarding the new LST reader.

Generally, the smaller a PR is the easier and quicker it should be to merge it, i.e. the more topic one mixes within one PR the longer it takes to discuss it.

In this specific case it is even more difficult. ctapipe would like to move these optional dependencies, like the SST1M, the Target and also the LST into their own packages, which will then be plugins for ctapipe.

For you as the author of the reader this means you have complete control over your own plugin package.
I will soonish start to make an examples based on the SST1M reader so people can have a look at it.

@dneise
Copy link
Member

dneise commented Jan 22, 2019

@FrancaCassol The tests fail .. but I do not know why.

There seems to be an issue with the order(?) of tests, I believe. I can re-create the failing tests on my laptop when I do just:

pytest

but when I only execute the failing test file like this:

pytest ctapipe/image/tests/test_waveform_cleaning.py

then the tests do not fail. This is very puzzling for me. Maybe you have an idea where this issue might come from?

@FrancaCassol
Copy link
Contributor Author

FrancaCassol commented Jan 22, 2019

Hi Dominik,

Generally, the smaller a PR is the easier and quicker it should be to merge it, i.e. the more topic one mixes within one PR the longer it takes to discuss it.

I agree totally with you, but I think the problem is the time that a PR takes to be accepted. I was waiting for this PR to be accepted before asking a new one, but now I have such an amount of new l (initially small) changes that the next PR will be again rather important. I even wonder if it make sense to continue with the present one because it is outdated with respect to my code... should I better start with a new one?

In this specific case it is even more difficult. ctapipe would like to move these optional dependencies, like the SST1M, the Target and also the LST into their own packages, which will then be plugins for ctapipe.

For you as the author of the reader this means you have complete control over your own plugin package.

This is interesting ... are we going to discuss on that at the next A&S WG meeting?

I will soonish start to make an examples based on the SST1M reader so people can have a look at it.

Wonderful :-)

@FrancaCassol
Copy link
Contributor Author

then the tests do not fail. This is very puzzling for me. Maybe you have an idea where this issue might come from?

For me it happens the same ... strange, it looks like the example_event changes ...

@kosack
Copy link
Contributor

kosack commented Jan 25, 2019

Yes, sorry this PR is being slow mainly because there is current work to move all EventSource "plugins" (e.g. those that need optional external libraries like Zfits) to separate repos, to make testing faster in the main ctapipe repo, and to avoid having to install so many dependencies. So soon this code will be external (probably a single module like ctapipe_io_zfits that contains both lst and nectarcam readers).

@FrancaCassol
Copy link
Contributor Author

Hi Karl,
let's say that this is the present source of delay ;-)

Just to be sure to understand: the message is: "wait till a bit"?

@FrancaCassol
Copy link
Contributor Author

It was decided to perform a successive PR, when the code will be more advanced and tested in the context of the lst-chain repository.

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