New calibration data tree and onsite scripts #724
Conversation
…alibraiton systematic corrections
There was a problem hiding this comment.
Apart from the small comments, the thing that should be changed in my opinion is how the lstchain version is obtained. Especially when you are working with a fixed tag, the sub-version will not be available and the script will fail.
Side note, I've not been able to produce ffactor_systematics files with lstchain/scripts/onsite/onsite_create_ffactor_systematics_file.py but I guess it is a matter of setting properly the configuration file.
| @@ -21,8 +22,9 @@ | |||
|
|
|||
| required.add_argument('-r', '--run_number', help="Run number with drs4 pedestals", | |||
| type=int, required=True) | |||
| version,subversion=lstchain.__version__.rsplit('.post',1) | |||
There was a problem hiding this comment.
This causes trouble if the lstchain version is just a simple tag as 0.7.5 without subversion:
>>> lstchain.__version__
'0.7.5'
>>> version,subversion=lstchain.__version__.rsplit('.post',1)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: not enough values to unpack (expected 2, got 1)Something like this could do the trick:
try:
version, subversion=lstchain.__version__.rsplit('.post',1)
except ValueError:
version = lstchain.__version__There was a problem hiding this comment.
Ops, I just modified in a different way, let me know if it is fine
lstchain/scripts/onsite/onsite_create_ffactor_systematics_file.py
Outdated
Show resolved
Hide resolved
lstchain/scripts/onsite/onsite_create_calibration_files_with_batch.py
Outdated
Show resolved
Hide resolved
lstchain/scripts/onsite/onsite_create_calibration_files_with_batch.py
Outdated
Show resolved
Hide resolved
| cmd = f"srun onsite_create_calibration_file -r {run} " \ | ||
| f"-p {ped_run} -v {prod_id} --sub_run {sub_run} " \ | ||
| f"-b {base_dir} -s {stat_events} --output_base_name {output_base_name} " \ | ||
| f"--filters {filters} --sys_date {sys_date} " \ | ||
| f"--config {config_file} --time_run {time_run}" | ||
|
|
||
| if no_sys_correction: | ||
| cmd += " --no_sys_correction" | ||
|
|
||
| fh.write(cmd) |
There was a problem hiding this comment.
Maybe, it'd be good to propagate the return code beyond the slurm job pilot. In this way, you will actually know whether the job finished successfully or not. For example, if you do not use the no_sys_correction the first time the job will raise an IOError but according to slurm the exit code is 0 as it had finished with no problems. Maybe it has to do with the way in which the error is raised in python?
There was a problem hiding this comment.
Yes, it would be nice, do you perhaps know how to do it?
There was a problem hiding this comment.
The problem is that you run a batch command and the submission was successful, the job itself failed.
To keep track of which jobs failed and why, you need a much more complex system, that e.g. stores submitted jobs in a database and periodically checks if the job succeeded or not.
There was a problem hiding this comment.
yes, and with slurm is not so easy because the error files are always produced and some time with not meaningful errors
There was a problem hiding this comment.
You should make sure that stdout and stderr are going into the same file and that exit codes are correctly propagated in the scripts. E.g. bash scripts should always have set -euo pipefail at the start.
There was a problem hiding this comment.
why in the same file?
…-lstchain into calibration_new_tree
|
Dear @maxnoe and @morcuended, |
There was a problem hiding this comment.
Thanks for the implementation @FrancaCassol . I do not see any major problem with it, but I left a few suggestions that you might want to have a look at
FrancaCassol
dismissed stale reviews from jsitarek and morcuended
via
a41ceb6
…h and are coeherent with a F-factor systematics correction at code calibraiton level
|
Dear @moralejo and @rlopezcoto, It seems to me this code is mature to be merged. Before including it in a production release, we must setup the calibration tree in the /fefs/aswg/data/real/monitoring/ directory. In plan to do it as soon as possible with help of @morcuended, so that he can then proceed with a small test production. |
This PR includes the following points:
A schematic description of the data tree and of the scripts is given in this note:
CameraCalibrationNote.pdf