Set the default timestamps in config file

[morcuended]

In order to be able to use any of the three available timestamps for pointing interpolation, I propose these changes (suggested by @rlopezcoto). I do not know if the nomenclature is the most convenient or not.

• Add the timestamps_pointing variable in the standard config file
• Use the timestamps defined in the config file (either ucts, dragon or tib) for pointing interpolation instead of having one of them hardcoded.

In this way, it will be possible to analyze old data using the required timestamps without having to change the code each time.

[rlopezcoto]

Thanks @morcuended!
Just for the record and what you told us by mail: this addition to the config file will allow to use dragon time for the data taken in November and ucts in the one taken from now on.

[morcuended]

Thanks @morcuended!
Just for the record and what you told us by mail: this addition to the config file will allow to use dragon time for the data taken in November and ucts in the one taken from now on.

Yes, that's it

Commenting it with @labsaha we realized that probably is better to define three variables in the config file like:

"ucts_timestamps_pointing": "true"
"dragon_timestamps_pointing": "false"
"tib_timestamps_pointing": "false"

because one might not know the names of the variables (ucts, dragon, tib), which are only written in the dl0_to_dl1.py script. Thus the code would instead look like this:

# Select the timestamps to be used for pointing interpolation
if config['ucts_timestamps_pointing']:
    event_timestamps = ucts_time                                                                                     
elif config['dragon_timestamps_pointing']:                                                                    
    event_timestamps = dragon_time                                                                                      
elif config['tib_timestamps_pointing']:                                                                        
    event_timestamps = tib_time   

What do you think @rlopezcoto?

[rlopezcoto]

I think it is a good idea, but I also see it a bit dangerous because if more than one of the variables are set as true in the config file, the selection may be overwritten. You can maybe add a line throwing an error if more than one of the time stamps have been selected.

[vuillaut]

I think it is a good idea, but I also see it a bit dangerous because if more than one of the variables are set as true in the config file, the selection may be overwritten. You can maybe add a line throwing an error if more than one of the time stamps have been selected.

I agree with @rlopezcoto here.
In this case, we don't know what is the default one in case everything is set to True (by mistake).

The general question is "should the standard config file be exhaustive about the options?"
to which I would reply no and that this is the role of the documentation. But ahem, you see where this is getting us, that would require a documentation 🙃

(note that this question is not limited to the option at hand here)

ps: I would personally keep the current impementation, but it's as you prefer, both options have their flaws.

[vuillaut]

else:
    raise ValueError("the timestamps_pointing option is not a valid one")

[morcuended]

Thanks, @vuillaut!
The error should be raised whenever there is more than one timestamp option set as true, right? So first we have to check this.

We were thinking of rather creating a timestamps_pointing dictionary within config file. It would look like this:

"timestamps_pointing":{
    "ucts": true
    "dragon": false
    "tib": false
}

and check whether there is more than one true element with something like this:
list(filter(config["timestamps_pointing"].get, config["timestamps_pointing"]))
So it would be:

if len(list(filter(config["timestamps_pointing"].get,
                   config["timestamps_pointing"]))) > 1:
    raise ValueError("the timestamps_pointing option is not a valid one")

But it's quite the opposite of explicit. What do you think?

[morcuended]

else:
    raise ValueError("the timestamps_pointing option is not a valid one")

OK, I didn't quite understand yesterday when I saw it first. Now I also prefer the first implementation with the addition of these (else & raise) lines. Makes the code clearer. With the second implementation, it wouldn't be so clear how to raise the error, because it has to be checked whether there is only one true option or not.

[rlopezcoto]

ok, everything looks good now, thanks @morcuended and @vuillaut! If nobody else opposes, I'll also merge