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

Oot pileup boostio #3806

Merged
merged 3 commits into from May 15, 2014
Merged

Conversation

igv4321
Copy link
Contributor

@igv4321 igv4321 commented May 11, 2014

HCAL out-of-time pileup subtraction code, using boost io for storing objects

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @igv4321 for CMSSW_7_1_X.

Oot pileup boostio

It involves the following packages:

CondCore/HcalPlugins
CondFormats/DataRecord
CondFormats/HcalObjects
RecoLocalCalo/HcalRecAlgos
RecoLocalCalo/HcalRecProducers

@apfeiffer1, @diguida, @cmsbuild, @anton-a, @thspeer, @rcastello, @slava77, @ggovi, @Degano, @nclopezo can you please review it and eventually sign? Thanks.
@ghellwig, @argiro 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

@slava77
Copy link
Contributor

slava77 commented May 12, 2014

Hi Igor,

Please post some details to document the changes (best to be in the PR description field, up at the top):

  • are there changes to default physics outputs
  • some link to slides describing the implementation

Thank you

@igv4321
Copy link
Contributor Author

igv4321 commented May 12, 2014

Hi Slava,

There may be very minor changes to default physics outputs.
Some variables that were floats became doubles, so the round-off
errors are slightly different.

Perhaps, the best reference which describes the algorithm is
https://indico.cern.ch/event/312786/contribution/3/material/slides/0.pdf

Regards,
Igor

On 05/12/2014 09:15 AM, Slava Krutelyov wrote:

Hi Igor,

Please post some details to document the changes (best to be in the PR description field, up at the top):

  • are there changes to default physics outputs
  • some link to slides describing the implementation

Thank you


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

@apfeiffer1
Copy link
Contributor

+1
looks OK.

@@ -0,0 +1,80 @@
#ifndef RecoLocalCalo_HcalRecAlgos_BoostIODBReader_h
Copy link
Contributor

Choose a reason for hiding this comment

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

The reader and the writer should be removed from HcalRecAlgos. Please, move to /test if really necessary to be here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Slava,

I don't think this should be in "test". This code can be used
with arbitrary objects serializable by boost and with arbitrary
record classes. I am sure it will be useful in the future to
instantiate these templates with other template parameters.
Since the code is header-only, it does not affect linking
dependencies of RecoLocalCalo/HcalRecAlgos sources.

And, of course, at this point this code has direct relationship
to Hcal algorithms because it is used to store and retrieve OOT
pileup correction objects.

Regards,
Igor

On 05/13/2014 05:33 PM, Slava Krutelyov wrote:

In RecoLocalCalo/HcalRecAlgos/interface/BoostIODBReader.h:

@@ -0,0 +1,80 @@
+#ifndef RecoLocalCalo_HcalRecAlgos_BoostIODBReader_h

The reader and the writer should be removed from HcalRecAlgos. Please, move to /test if really necessary to be here.


Reply to this email directly or view it on GitHub https://github.com/cms-sw/cmssw/pull/3806/files#r12612585.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not going to be run in regular production application, only by experts once in a blue moon.
This is pretty much the definition of what should be in the test.
This comment refers to the instances defined in the plugin as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Slava,

While this code will not be run in regular production application,
it will be used to prepare production databases.

In principle, I agree that there is probably a better place for the
DB read/write headers. However, it is surely not "test". Why don't
we keep it as it is for pre8 (since everything works, and the deadline
is fast approaching), and I will find a better place for it before pre9.

Regards,
Igor

On 05/13/2014 09:37 PM, Slava Krutelyov wrote:

In RecoLocalCalo/HcalRecAlgos/interface/BoostIODBReader.h:

@@ -0,0 +1,80 @@
+#ifndef RecoLocalCalo_HcalRecAlgos_BoostIODBReader_h

It's not going to be run in regular production application, only by experts once in a blue moon.
This is pretty much the definition of what should be in the test.
This comment refers to the instances defined in the plugin as well.


Reply to this email directly or view it on GitHub https://github.com/cms-sw/cmssw/pull/3806/files#r12618804.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems these should be moved to a Cond_/_/bin/ directory - has nothing to do with reconstruction indeed.

Copy link
Contributor

Choose a reason for hiding this comment

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

and due to their current location we have an "Algos" package with a plugins directory in addition to the corresponding producers package. Always good. Anyway, please fix this week.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi!
What about CondTools/Hcal?

@slava77
Copy link
Contributor

slava77 commented May 13, 2014

What are these about?
(this is apparently not related to the OOT corrections. Still, the first time I notice this kind of warnings. Should I just ignore?)

[2014-05-14 00:10:36,425] WARNING: No non-transient members found for serializable class HcalPFCorrs
[2014-05-14 00:10:36,425] WARNING: No non-transient members found for serializable class HcalLongRecoParams
[2014-05-14 00:10:36,427] WARNING: No non-transient members found for serializable class HcalRecoParams
[2014-05-14 00:10:36,427] WARNING: No non-transient members found for serializable class HcalValidationCorrs
[2014-05-14 00:10:36,427] WARNING: No non-transient members found for serializable class HcalQIEData
[2014-05-14 00:10:36,427] WARNING: No non-transient members found for serializable class HcalGains
[2014-05-14 00:10:36,427] WARNING: No non-transient members found for serializable class HcalRespCorrs
[2014-05-14 00:10:36,428] WARNING: No non-transient members found for serializable class HcalMCParams
[2014-05-14 00:10:36,428] WARNING: No non-transient members found for serializable class HcalZSThresholds
[2014-05-14 00:10:36,428] WARNING: No non-transient members found for serializable class HcalChannelQuality
[2014-05-14 00:10:36,428] WARNING: No non-transient members found for serializable class HcalLUTCorrs
[2014-05-14 00:10:36,428] WARNING: No non-transient members found for serializable class HcalCondObjectContainerBase
[2014-05-14 00:10:36,428] WARNING: No non-transient members found for serializable class HcalGainWidths
[2014-05-14 00:10:36,428] WARNING: No non-transient members found for serializable class HcalTimeCorrs
[2014-05-14 00:10:36,429] WARNING: No non-transient members found for serializable class HcalCalibrationQIEData
[2014-05-14 00:10:36,429] WARNING: No non-transient members found for serializable class HcalTimingParams
[2014-05-14 00:10:36,429] WARNING: No non-transient members found for serializable class HcalFlagHFDigiTimeParams


@igv4321
Copy link
Contributor Author

igv4321 commented May 13, 2014

I have not seen these warnings in CMSSW_7_1_X_2014-05-10-1400
which I was using to test the code (and, surely, they are not related
to OOT pileup classes). Andreas, could you please comment on these?

Thanks,
Igor

On 05/13/2014 05:36 PM, Slava Krutelyov wrote:

What are these about?
(this is apparently not related to the OOT corrections. Still, the first time I notice this kind of warnings. Should I just ignore?)

|[2014-05-14 00:10:36,425] WARNING: No non-transient members found for serializable class HcalPFCorrs
[2014-05-14 00:10:36,425] WARNING: No non-transient members found for serializable class HcalLongRecoParams
[2014-05-14 00:10:36,427] WARNING: No non-transient members found for serializable class HcalRecoParams
[2014-05-14 00:10:36,427] WARNING: No non-transient members found for serializable class HcalValidationCorrs
[2014-05-14 00:10:36,427] WARNING: No non-transient members found for serializable class HcalQIEData
[2014-05-14 00:10:36,427] WARNING: No non-transient members found for serializable class HcalGains
[2014-05-14 00:10:36,427] WARNING: No non-transient members found for serializable class HcalRespCorrs
[2014-05-14 00:10:36,428] WARNING: No non-transient members found for serializable class HcalMCParams
[2014-05-14 00:10:36,428] WARNING: No non-transient members found for serializable class HcalZSThresholds
[2014-05-14 00:10:36,428] WARNING: No non-transient members found for serializable class HcalChannelQuality
[2014-05-14 00:10:36,428] WARNING: No non-transient members found for serializable class HcalLUTCorrs
[2014-05-14 00:10:36,428] WARNING: No non-transient members found for serializable class HcalCondObjectContainerBase
[2014-05-14 00:10:36,428] WARNING: No non-transient members found for serializable class HcalGainWidths
[2014-05-14 00:10:36,428] WARNING: No non-transient members found for serializable class HcalTimeCorrs
[2014-05-14 00:10:36,429] WARNING: No non-transient members found for serializable class HcalCalibrationQIEData
[2014-05-14 00:10:36,429] WARNING: No non-transient members found for serializable class HcalTimingParams
[2014-05-14 00:10:36,429] WARNING: No non-transient members found for serializable class HcalFlagHFDigiTimeParams

|
|


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

@slava77
Copy link
Contributor

slava77 commented May 14, 2014

+1

for #3806 06cd27e

tested in CMSSW_7_1_X_2014-05-13-1400 (test area sign374)

no differences in HCAL hits beyond numerical precision, as expected

CPU looks OK (unchanged)

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_1_X IBs unless changes (tests are also fine). @nclopezo, @ktf can you please take care of it?

@apfeiffer1
Copy link
Contributor

Hi Igor,

these warnings come from the (new) script which runs on CondFormats packages to generate automatically the serialisation code.

As you do not use the macro (COND_SERIALIZABLE;) in your header files, but explicitly add the declaration “manually" (and the implementation in the sra/...cc files), the script cannot find anything to be serialised and prints the warning (which can be ignored as you handle things explicitly).

cheers, andreas

PS: The instructions on how to prepare new conditions classes in the “official” cmssw way are at:
https://twiki.cern.ch/twiki/bin/viewauth/CMS/NewCondCore2013

On 14 May 2014, at 01:11, igv4321 notifications@github.com wrote:

I have not seen these warnings in CMSSW_7_1_X_2014-05-10-1400
which I was using to test the code (and, surely, they are not related
to OOT pileup classes). Andreas, could you please comment on these?

Thanks,
Igor

On 05/13/2014 05:36 PM, Slava Krutelyov wrote:

What are these about?
(this is apparently not related to the OOT corrections. Still, the first time I notice this kind of warnings. Should I just ignore?)

|[2014-05-14 00:10:36,425] WARNING: No non-transient members found for serializable class HcalPFCorrs
[2014-05-14 00:10:36,425] WARNING: No non-transient members found for serializable class HcalLongRecoParams
[2014-05-14 00:10:36,427] WARNING: No non-transient members found for serializable class HcalRecoParams
[2014-05-14 00:10:36,427] WARNING: No non-transient members found for serializable class HcalValidationCorrs
[2014-05-14 00:10:36,427] WARNING: No non-transient members found for serializable class HcalQIEData
[2014-05-14 00:10:36,427] WARNING: No non-transient members found for serializable class HcalGains
[2014-05-14 00:10:36,427] WARNING: No non-transient members found for serializable class HcalRespCorrs
[2014-05-14 00:10:36,428] WARNING: No non-transient members found for serializable class HcalMCParams
[2014-05-14 00:10:36,428] WARNING: No non-transient members found for serializable class HcalZSThresholds
[2014-05-14 00:10:36,428] WARNING: No non-transient members found for serializable class HcalChannelQuality
[2014-05-14 00:10:36,428] WARNING: No non-transient members found for serializable class HcalLUTCorrs
[2014-05-14 00:10:36,428] WARNING: No non-transient members found for serializable class HcalCondObjectContainerBase
[2014-05-14 00:10:36,428] WARNING: No non-transient members found for serializable class HcalGainWidths
[2014-05-14 00:10:36,428] WARNING: No non-transient members found for serializable class HcalTimeCorrs
[2014-05-14 00:10:36,429] WARNING: No non-transient members found for serializable class HcalCalibrationQIEData
[2014-05-14 00:10:36,429] WARNING: No non-transient members found for serializable class HcalTimingParams
[2014-05-14 00:10:36,429] WARNING: No non-transient members found for serializable class HcalFlagHFDigiTimeParams

|
|


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


Reply to this email directly or view it on GitHub.

@ktf
Copy link
Contributor

ktf commented May 14, 2014

these warnings come from the (new) script which runs on CondFormats
packages to generate automatically the serialisation code.
As you do not use the macro (COND_SERIALIZABLE;) in your header files,
but explicitly add the declaration “manually" (and the
implementation in the sra/...cc files), the script cannot find
anything to be serialised and prints the warning (which can be ignored
as you handle things explicitly).

Can we have a way to silence the warnings in case things are done in a
"non-official" way or can you simply do them using the official recipe,
please?

@apfeiffer1
Copy link
Contributor

On Wed, May 14, 2014 at 9:12 AM, Giulio Eulisse notifications@github.comwrote:

these warnings come from the (new) script which runs on CondFormats
packages to generate automatically the serialisation code.
As you do not use the macro (COND_SERIALIZABLE;) in your header files,
but explicitly add the declaration “manually" (and the
implementation in the sra/...cc files), the script cannot find
anything to be serialised and prints the warning (which can be ignored
as you handle things explicitly).

Can we have a way to silence the warnings in case things are done in a
"non-official" way

that would only work with a brute-force hack (a la "if packagename == ...")

or can you simply do them using the official recipe,

please?

this would be the best case - though then the manual serialisation code
needs to be protected by some kind of "if not building in cmssw" to avoid
duplicated symbols as the official recipe implies that the code will be
generated by the script.

cheers, andreas

@igv4321
Copy link
Contributor Author

igv4321 commented May 14, 2014

Hi Andreas,

Please note that these warnings are printed about classes
I have nothing to do with -- these are not "my" header files,
so I do not quite understand the logic of what the script
is doing.

And, of course, the original purpose of proceeding via
boost I/O was to have the same serialized format both in
and outside CMSSW (so that objects generated outside
CMSSW could be directly imported into the database).
I do not think we can ensure this kind of compatibility
by replacing manual serialization code with automatic -- the
order of serialized members, for example, is not guaranteed
to be the same.

Regards,
Igor

On 05/14/2014 02:38 AM, Andreas Pfeiffer wrote:

On Wed, May 14, 2014 at 9:12 AM, Giulio Eulisse notifications@github.comwrote:

these warnings come from the (new) script which runs on CondFormats
packages to generate automatically the serialisation code.
As you do not use the macro (COND_SERIALIZABLE;) in your header files,
but explicitly add the declaration “manually" (and the
implementation in the sra/...cc files), the script cannot find
anything to be serialised and prints the warning (which can be ignored
as you handle things explicitly).

Can we have a way to silence the warnings in case things are done in a
"non-official" way

that would only work with a brute-force hack (a la "if packagename == ...")

or can you simply do them using the official recipe,

please?

this would be the best case - though then the manual serialisation code
needs to be protected by some kind of "if not building in cmssw" to avoid
duplicated symbols as the official recipe implies that the code will be
generated by the script.

cheers, andreas


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

@apfeiffer1
Copy link
Contributor

Hi Igor,

Please note that these warnings are printed about classes
I have nothing to do with -- these are not "my" header files,
so I do not quite understand the logic of what the script
is doing.

OK, so these classes are not meant to be stored as conditions ?

And, of course, the original purpose of proceeding via

boost I/O was to have the same serialized format both in
and outside CMSSW (so that objects generated outside
CMSSW could be directly imported into the database).

yes, and you do not want the dependencies of the script in the "outside
world" , that's why I did not ask you to move :)

I do not think we can ensure this kind of compatibility
by replacing manual serialization code with automatic -- the
order of serialized members, for example, is not guaranteed
to be the same.

well, I certainly hope that you do not change the order of the members. ;)

The script will take the order as they are in the header file, so that way
the code is the same.

The present planning is that the script will in the future do checks and
ensure that things don't change.

Thanks,
cheers, andreas

@igv4321
Copy link
Contributor Author

igv4321 commented May 14, 2014

Hi Andreas,

The classes about which the warnings were generated are just
normal conditions that were in CondFormats/HcalObjects long
before introduction of boost I/O.

Yes, the dependencies of the script itself are rather heavy.
Depending just on boost and portable archive code is much
easier in the outside world -- boost is built into most Linux
distributions anyway, and portable archives are just a few files.

Of course, I do not intend to change the order of the members.
However, I do not quite understand how the version evolution
will work with the automatic script. With the manual serialization
it is, of course, easy to ensure that future versions of the class
could be deserialized from archives made with prior versions,
as boost.serialization was designed with this evolution in mind.

Regards,
Igor

On 05/14/2014 03:50 AM, Andreas Pfeiffer wrote:

Hi Igor,

Please note that these warnings are printed about classes
I have nothing to do with -- these are not "my" header files,
so I do not quite understand the logic of what the script
is doing.

OK, so these classes are not meant to be stored as conditions ?

And, of course, the original purpose of proceeding via

boost I/O was to have the same serialized format both in
and outside CMSSW (so that objects generated outside
CMSSW could be directly imported into the database).

yes, and you do not want the dependencies of the script in the "outside
world" , that's why I did not ask you to move :)

I do not think we can ensure this kind of compatibility
by replacing manual serialization code with automatic -- the
order of serialized members, for example, is not guaranteed
to be the same.

well, I certainly hope that you do not change the order of the members. ;)

The script will take the order as they are in the header file, so that way
the code is the same.

The present planning is that the script will in the future do checks and
ensure that things don't change.

Thanks,
cheers, andreas


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

@igv4321
Copy link
Contributor Author

igv4321 commented May 14, 2014

Hi Andreas,

In principle, if the warnings are indeed generated by the script when
COND_SERIALIZABLE macro is not present (although I am not sure
about validity of this explanation in the case of warnings described by
Slava), it should be also easy to check for the presence of declaration
"friend class boost::serialization::access" in the header. It would be even
more reliable to check whether the function "serialize" is already present,
but then one must also handle the case of split serialization with the
pair of "save" and "load" methods. Then the warnings can be disabled
just for the classes for which serialization code already exists.

Regards,
Igor

On 05/14/2014 02:38 AM, Andreas Pfeiffer wrote:

On Wed, May 14, 2014 at 9:12 AM, Giulio Eulisse notifications@github.comwrote:

these warnings come from the (new) script which runs on CondFormats
packages to generate automatically the serialisation code.
As you do not use the macro (COND_SERIALIZABLE;) in your header files,
but explicitly add the declaration “manually" (and the
implementation in the sra/...cc files), the script cannot find
anything to be serialised and prints the warning (which can be ignored
as you handle things explicitly).

Can we have a way to silence the warnings in case things are done in a
"non-official" way

that would only work with a brute-force hack (a la "if packagename == ...")

or can you simply do them using the official recipe,

please?

this would be the best case - though then the manual serialisation code
needs to be protected by some kind of "if not building in cmssw" to avoid
duplicated symbols as the official recipe implies that the code will be
generated by the script.

cheers, andreas


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

@igv4321
Copy link
Contributor Author

igv4321 commented May 14, 2014

Hi Giulio and Andreas,

I was able to reproduce the warnings that Slava saw by simply
doing the following on lxplus:

setenv SCRAM_ARCH slc6_amd64_gcc490
scram proj CMSSW_7_1_X_2014-05-14-0200
cd CMSSW_7_1_X_2014-05-14-0200/src
cmsenv
git cms-addpkg CondFormats/HcalObjects
scram b

This release, of course, does not yet have any code related to
HCAL OOT pileup subtraction in it, so it is an unrelated problem.

Since these warnings are a separate issue, I suggest merging
PR #3806 so that it gets into pre8.

Regards,
Igor

On 05/14/2014 04:23 AM, Igor Volobouev wrote:

Hi Andreas,

The classes about which the warnings were generated are just
normal conditions that were in CondFormats/HcalObjects long
before introduction of boost I/O.

Yes, the dependencies of the script itself are rather heavy.
Depending just on boost and portable archive code is much
easier in the outside world -- boost is built into most Linux
distributions anyway, and portable archives are just a few files.

Of course, I do not intend to change the order of the members.
However, I do not quite understand how the version evolution
will work with the automatic script. With the manual serialization
it is, of course, easy to ensure that future versions of the class
could be deserialized from archives made with prior versions,
as boost.serialization was designed with this evolution in mind.

Regards,
Igor

On 05/14/2014 03:50 AM, Andreas Pfeiffer wrote:

Hi Igor,

Please note that these warnings are printed about classes
I have nothing to do with -- these are not "my" header files,
so I do not quite understand the logic of what the script
is doing.

OK, so these classes are not meant to be stored as conditions ?

And, of course, the original purpose of proceeding via

boost I/O was to have the same serialized format both in
and outside CMSSW (so that objects generated outside
CMSSW could be directly imported into the database).

yes, and you do not want the dependencies of the script in the "outside
world" , that's why I did not ask you to move :)

I do not think we can ensure this kind of compatibility
by replacing manual serialization code with automatic -- the
order of serialized members, for example, is not guaranteed
to be the same.

well, I certainly hope that you do not change the order of the members. ;)

The script will take the order as they are in the header file, so that way
the code is the same.

The present planning is that the script will in the future do checks and
ensure that things don't change.

Thanks,
cheers, andreas


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

@apfeiffer1
Copy link
Contributor

Hi all,

finally I found the time to look into this.

The warning is not, as I thought before, coming from a class not having the
macro. The warning - as it clearly says - is there because the classes it
complains about do not have any non-transient data members, they are all
derived from some container class which contains the actual persistent
members, so each of the derived classes only has some added methods.

Giulio, if you want, I can "demote" these warnings to info, OTOH, they
indeed can point to trouble if someone defines a class to be persistent and
then forgets to add a member ... let me know.

Thanks,
cheers, andreas

davidlange6 added a commit that referenced this pull request May 15, 2014
@davidlange6 davidlange6 merged commit 40b4269 into cms-sw:CMSSW_7_1_X May 15, 2014
@ktf
Copy link
Contributor

ktf commented May 15, 2014

@apfeiffer1 IMHO:

  • If this is fatal, it should be an error.
  • If it needs to be fixed, it should stay a warning and you should provide information to @igv4321 on how to fix it.
  • If it is really "just in case", you should simply disable it, since as you know no one will ever look at it.

@apfeiffer1
Copy link
Contributor

Ciao Giulio,

@apfeiffer1 https://github.com/apfeiffer1 IMHO:

  • If this is fatal, it should be an error.
  • If it needs to be fixed, it should stay a warning and you should
    provide information to @igv4321 https://github.com/igv4321 on how to
    fix it.
  • If it is really "just in case", you should simply disable it, since
    as you know no one will ever look at it.

yessir !! ;)

It's the last one. Will do.

Thanks,
cheers, andreas

@ktf
Copy link
Contributor

ktf commented May 15, 2014

yessir !! ;)

.. and from now on the official language of CMS will be.. Italian.. mmm
wait.. ;)

@igv4321 igv4321 deleted the oot-pileup-boostio branch May 16, 2014 16:24
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

7 participants