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

New flexyble payloads for DT ttrig, vdrift and uncertainties #5977

Merged
merged 1 commit into from Oct 30, 2014

Conversation

namapane
Copy link
Contributor

New payload format for DT ttrig, vdrift and uncertainties, to allow for additional flexibility, e.g. introduce dependencies from track angle, position in the chamber etc. Some of these dependencies are hardcoded corrections in the code at the moment, or cannot be implemented at all.

This PR just includes payload definition, records, and tools to read/write the new format. It includes no change in the existing format, nor any usage of the new format in reconstruction code.

Reference: DT report at the DPG coordination meeting, Oct, 25:
https://indico.cern.ch/event/346292/contribution/4/material/slides/0.pdf

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @namapane (Nicola Amapane) for CMSSW_7_3_X.

New flexyble payloads for DT ttrig, vdrift and uncertainties

It involves the following packages:

CalibMuon/DTCalibration
CondCore/DTPlugins
CondFormats/DTObjects
CondFormats/DataRecord

@apfeiffer1, @nclopezo, @cerminar, @cmsbuild, @diguida, @rcastello, @ggovi, @mmusich can you please review it and eventually sign? Thanks.
@ghellwig this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@nclopezo, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@davidlange6
Copy link
Contributor

hold

@apfeiffer1 - I can't tell if you comment is an agreement or a proposal for an agreement with @namapane and other contributors to the PR. Could you clarify. Thanks.

@namapane
Copy link
Contributor Author

Dear Andreas,

Of course we are responsible of any required forward an backward
compatibility, but I'm not 100% sure of what cases you have in mind:

We have defined new payloads format with their new records, we won't mix
old records and new payload formats.

Old releases will have only old payloads, associated to the old records.

New releases will have a port of the old payloads into the new one. They
could possibly also include the old payloads associated with the old
records, the two are completely independent, but I see no point in this.

I also see no point in backporting new calibrations to old releases, but
should this be needed, we would use the old payload formats.
Of course it is not possible to backport new features that are
introduced with the new format (like handling dependencies), but this is
always the case, if you develop a reconstruction that uses a finer
calibration technique, you need to backport the code to have it in an
older release.

So what case are you specifically referring to?

Cheers
Nicola

On 28-Oct-14 11:48, Andreas Pfeiffer wrote:

+1
conditional approval: it is understood that the sole responsibility for
ensuring both forward and backward compatibility of the data [1] is with
the owner of the payloads and not the DB team.

Thanks,
cheers, andreas

[1] the owner has to ensure that any previous version of the code in
future "old" releases can always correctly read the data even if the data
is changed and written by a more recent future release. So, for example,
data written with 8.1.0 can always be correctly read even by code in
7.3.0 (without any change in that release).


Reply to this email directly or view it on GitHub
#5977 (comment).

@apfeiffer1
Copy link
Contributor

@davidlange6, Giacomo discussed this with Nicola and came to this agreement. I just wanted to put it in here for reference to clarify this point.

@apfeiffer1
Copy link
Contributor

@namapane, I was not talking about back-porting to any release which exists already, instead I was referring to the future. In a year or so you might want to change the payload to some other content, and it is then (hence my reference to 8.1.0 in the example) when you will need to make sure that "old" releases at that time (earlier than the one where you want the then new class go in -- like the 7.3.0 in my example, which will be the first one with the new class) can still read the then new stuff. I was not talking about 7.2.x or older as I assumed that you won't need the new classes there.

Hope that clarifies the issue.

@namapane
Copy link
Contributor Author

Ah ok sure. Yes we will take care of any future backward compatibility
needs.

In any case we think that this change will cover the future needs of the
entire Run II at least.

Cheers,
Nicola

On 28-Oct-14 13:22, Andreas Pfeiffer wrote:

@namapane https://github.com/namapane, I was not talking about
back-porting to any release which exists already, instead I was
referring to the future. In a year or so you might want to change the
payload to some other content, and it is /then/ (hence my reference to
8.1.0 in the example) when you will need to make sure that "old"
releases /at that time/ (earlier than the one where you want the /then/
new class go in -- like the 7.3.0 in my example, which will be the first
one with the new class) can still read the /then/ new stuff. I was not
talking about 7.2.x or older as I assumed that you won't need the new
classes there.

Hope that clarifies the issue.


Reply to this email directly or view it on GitHub
#5977 (comment).

@davidlange6
Copy link
Contributor

@diguida - do you have remaining issues with this PR?

@diguida
Copy link
Contributor

diguida commented Oct 30, 2014

+1
sorry for overlooking it...
We share the concern by @apfeiffer1 especially for the future support of the current data hosted by condition objects currently in release/GT.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_3_X IBs unless changes (tests are also fine). This pull request will be automatically merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants