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

Add time calibration script #261

Merged
merged 1 commit into from
Jan 16, 2020

Conversation

FrancaCassol
Copy link
Collaborator

I add the script to calculate the time calibration which was missing by mistake in the PR #248

Copy link
Contributor

@rlopezcoto rlopezcoto left a comment

Choose a reason for hiding this comment

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

thanks @FrancaCassol! Just a small comment


# Optional argument
parser.add_argument("--max_events",
help="Maximum numbers of events to read. Default = 20000",
Copy link
Contributor

Choose a reason for hiding this comment

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

is this the default number of events that you are using? Or are you using None to read them all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is the default value, I guess @pawel21 suggested it because it is enough to get a good estimation

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, thanks, @pawel21 could you confirm that this is enough to get this accepted?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@rlopezcoto , Yes, this should be enough to fill all bins in relative short time.

@FrancaCassol
Copy link
Collaborator Author

Hi @rlopezcoto, can we merge this?
Otherwise the calibration onsite script is not working

@rlopezcoto rlopezcoto merged commit d0dfff8 into cta-observatory:master Jan 16, 2020
@FrancaCassol FrancaCassol deleted the add_time_script branch January 16, 2020 15:03
@morcuended
Copy link
Member

I tried to run the script with the default config (max_events=20000) for several of the last runs. I got this error message.

...
    raise RuntimeError("Not enough events to coverage all capacitor. "
RuntimeError: Not enough events to coverage all capacitor. Please use more events to time calibration file.

@pawel21
Copy link
Collaborator

pawel21 commented Jan 16, 2020

Dear @morcuended , I have checked this data, seems that calibration pulse in few pixels are missing, so script raise error. You can use data from November/December 2019.

@FrancaCassol
Copy link
Collaborator Author

Dear @pawel21, all,
I think this must be more strictly defined.

I am afraid it will often happen that one or more pixels are off, we can not leave to the shifter the search for an old time calibration file. If the pixels are off why should not the script work anyway for the other ones?

More generally speaking: How often must the time calibration coefficients be calculated? If the calculation is daily, the script must be robust enough to work with all pixel conditions, instead if we can calculate them on periodically good selected runs, I prefer to take it out from the onsite script

@moralejo
Copy link
Collaborator

moralejo commented Jan 17, 2020 via email

@pawel21
Copy link
Collaborator

pawel21 commented Jan 17, 2020

Dear Franca,
I think we don't need calcurate calibration coefficients every night. In MAGIC we use one time calibrattion file for 2 month or more. Offline, I will check stability of corrections.

@FrancaCassol
Copy link
Collaborator Author

I see.

@moralejo, we should then decide which protocol to follow to create these files: how often they must be created and where they must be stored so that the analysis scripts find them automatically. Then, somebody should be officially in charge for their periodical update

@pawel21, in any case, please make the script robust with respect to not working pixels.

@moralejo
Copy link
Collaborator

moralejo commented Jan 17, 2020 via email

@pawel21
Copy link
Collaborator

pawel21 commented Jan 17, 2020

Dear @moralejo ,
Today is not possible to prepare such script, I will do it in next week. I am sorry.
This affect only time correction, not another drs4 correction.

@FrancaCassol
Copy link
Collaborator Author

@pawel21 https://github.com/pawel21, when do you think the modifications to distinguish between non-working pixels and sparse DRS4 coverage can be done? If not possible today (let us know), let's not delay the lstchain release. We can use the old file Pawel suggested in the meantime (I understand this would only affect the time calibration and not the other DRS4 calibrations, correct?).

Could we then define where must be located this "service" calibration file in the data tree?

├── real
│   │
│   │   ├── calibration
│   │   │       └── <date>
│   │   │           └── prod_ID

Should we create a svc calibration subdirectory ? @vuillaut do you have any suggestion?

@pawel could you please provide a recent good (full camera) time calibration file?

@pawel21
Copy link
Collaborator

pawel21 commented Jan 17, 2020

I am sorry, . I don't have access to a computer until Sunday. @FrancaCassol could you produce time calibration file using data from run 1625 (2019/11/24, example notebook time_calibration) I would be grateful. Let me know in case of any problems.

@FrancaCassol
Copy link
Collaborator Author

Hi @pawel21,
yes, I can do it, but we need to decide where to put.

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.

5 participants