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.

Sign up for GitHub

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

Jump to bottom

Prepare for taking MF configurations from DB #6514

Closed
wants to merge 3 commits into from
Closed

Prepare for taking MF configurations from DB #6514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1b286d0
Create payloads for standard configurations
namapane Nov 17, 2014
57e928e
Fix for sorting volumes in phi, needed for numerical stability with t…
namapane Nov 19, 2014
8efe88d
minor fixes
namapane Nov 20, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 31 additions & 0 deletions CondFormats/MFObjects/test/writeAllMagFieldConfigDB.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#!/usr/bin/python

import os
import sys

SETUPS = ('71212', '', ('0T','2T','3T','3_5T','4T')), \
('90322', '2pi_scaled', (['3_8T'])), \
('120812', 'Run1', (['3_8T'])), \
('120812', 'Run2', (['3_8T'])), \
('130503', 'Run1', ('3_5T','3_8T')), \
('130503', 'Run2', ('3_5T','3_8T')),


for SETUP in SETUPS :
SET = SETUP[0]
SUBSET = SETUP[1]
for B_NOM in SETUP[2] :
print SET, SUBSET, B_NOM
sys.stdout.flush()
namespace = {'SET':SET, 'SUBSET':SUBSET, 'B_NOM':B_NOM}
execfile("writeMagFieldConfigDB.py",namespace)
process = namespace.get('process')
cfgFile = open('run.py','w')
cfgFile.write( process.dumpPython() )
cfgFile.write( '\n' )
cfgFile.close()
os.system("cmsRun run.py")
del namespace
del process
print ""

42 changes: 26 additions & 16 deletions CondFormats/MFObjects/test/writeMagFieldConfigDB.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,21 @@
import os

#SET = "71212"
SET = "90322"
#SET = "90322"
#SET = "120812"
#SET = "130503"

#SUBSET = ""
SUBSET = "2pi_scaled"
#SUBSET = "2pi_scaled"
#SUBSET = "Run1"
#SUBSET = "Run2"

# B_NOM = "0T"
# B_NOM = "2T"
# B_NOM = "3T"
# B_NOM = "3_5T"
B_NOM = "3.8T"
# B_NOM = "4T"
#B_NOM = "0T"
#B_NOM = "2T"
#B_NOM = "3T"
#B_NOM = "3_5T"
#B_NOM = "3.8T"
#B_NOM = "4T"


process = cms.Process("DumpToDB")
Expand Down Expand Up @@ -62,7 +62,7 @@ def createMetadata(aTag,aComment):
txtfile.write('{\n')
txtfile.write(' "destinationDatabase": "oracle://cms_orcoff_prep/CMS_COND_GEOMETRY",\n')
txtfile.write(' "destinationTags": {\n')
txtfile.write(' '+TAG+'": {\n')
txtfile.write(' "'+TAG+'": {\n')
txtfile.write(' "dependencies": {},\n')
txtfile.write(' "synchronizeTo": "offline"\n')
txtfile.write(' }\n')
Expand All @@ -82,14 +82,20 @@ def createMetadata(aTag,aComment):
'3T': 'grid_1103l_071212_3t',
'3_5T': 'grid_1103l_071212_3_5t',
#'3.8T': 'grid_1103l_071212_3_8t', #Deprecated
'4T': 'grid_1103l_071212_4t'}
'4T': 'grid_1103l_071212_4t'}
param = {'0T': 0.,
'2T': 2.,
'3T': 3.,
'3_5T': 3.5,
#'3_8T': 3.8,
'4T': 4.}
process.dumpToDB = cms.EDAnalyzer("MagFieldConfigDBWriter",
scalingVolumes = cms.vint32(),
scalingFactors = cms.vdouble(),
version = cms.string(versions[B_NOM]),
geometryVersion = cms.int32(90322),
paramLabel = cms.string('OAE_1103l_071212'),
paramData = cms.vdouble(3.8),
paramData = cms.vdouble(param[B_NOM]),
gridFiles = cms.VPSet(
cms.PSet( # Default tables, replicate sector 1
volumes = cms.string('1-312'),
Expand All @@ -99,10 +105,12 @@ def createMetadata(aTag,aComment):
)
)
)

if B_NOM == '0T' :
process.dumpToDB.paramLabel = 'Uniform'


if SET=="90322" :
if B_NOM!="3.8T" or SUBSET!="2pi_scaled": raise NameError("configuration invalid: "+SET)
if B_NOM!="3_8T" or SUBSET!="2pi_scaled": raise NameError("configuration invalid: "+SET)

from MagneticField.Engine.ScalingFactors_090322_2pi_090520_cfi import *

Expand Down Expand Up @@ -155,7 +163,7 @@ def createMetadata(aTag,aComment):


elif SET=="120812" :
if B_NOM!="3.8T" : raise NameError("B_NOM invalid for SET "+SET)
if B_NOM!="3_8T" : raise NameError("B_NOM invalid for SET "+SET)
if SUBSET=="Run1" : VERSION = 'grid_120812_3_8t_v7_small'
elif SUBSET=="Run2" : VERSION = 'grid_120812_3_8t_v7_large'
else : raise NameError("invalid SUBSET: "+SUBSET+ " for "+TAG )
Expand Down Expand Up @@ -216,7 +224,9 @@ def createMetadata(aTag,aComment):

elif SET=="130503" :
versions = {'3_5T': 'grid_130503_3_5t_v9',
'3.8T': 'grid_130503_3_8t_v9'}
'3_8T': 'grid_130503_3_8t_v9'}
param = {'3_5T': 3.5,
'3_8T': 3.8}

if SUBSET=="Run1" : VERSION = versions[B_NOM]+'_small'
elif SUBSET=="Run2" : VERSION = versions[B_NOM]+'_large'
Expand All @@ -228,7 +238,7 @@ def createMetadata(aTag,aComment):
version = cms.string(VERSION),
geometryVersion = cms.int32(130503),
paramLabel = cms.string('OAE_1103l_071212'),
paramData = cms.vdouble(3.8),
paramData = cms.vdouble(param[B_NOM]),

gridFiles = cms.VPSet(
# Volumes for which specific tables are used for each sector
Expand Down
123 changes: 123 additions & 0 deletions MagneticField/Engine/test/testMagneticFieldDB.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
#

import FWCore.ParameterSet.Config as cms

process = cms.Process("MAGNETICFIELDTEST")

process.source = cms.Source("EmptySource")
process.maxEvents = cms.untracked.PSet(
input = cms.untracked.int32(1)
)

process.load("MagneticField.Engine.volumeBasedMagneticFieldFromDB_cfi")

process.load("Configuration/StandardSequences/FrontierConditions_GlobalTag_cff")
from Configuration.AlCa.GlobalTag import GlobalTag
process.GlobalTag = GlobalTag(process.GlobalTag, 'auto:mc', '')



process.GlobalTag.toGet = cms.VPSet(
# Geometries
cms.PSet(record = cms.string("MFGeometryFileRcd"),
# tag = cms.string("MagneticFieldGeometry_90322"),
# connect = cms.untracked.string("sqlite_file:DB_Geom/mfGeometry_90322.db"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to keep all the commented out code? If not needed - should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I need all the commented code. This is a test .py, what's the problem here?

tag = cms.string("MFGeometry_90322"),
connect = cms.untracked.string("frontier://FrontierPrep/CMS_COND_GEOMETRY"),
label = cms.untracked.string("90322")
),
cms.PSet(record = cms.string("MFGeometryFileRcd"),
# tag = cms.string("MagneticFieldGeometry_120812"),
# connect = cms.untracked.string("sqlite_file:DB_Geom/mfGeometry_120812.db"),
tag = cms.string("MFGeometry_120812"),
connect = cms.untracked.string("frontier://FrontierPrep/CMS_COND_GEOMETRY"),
label = cms.untracked.string("120812")
),
cms.PSet(record = cms.string("MFGeometryFileRcd"),
# tag = cms.string("MagneticFieldGeometry_130503"),
# connect = cms.untracked.string("sqlite_file:DB_Geom/mfGeometry_130503.db"),
tag = cms.string("MFGeometry_130503"),
connect = cms.untracked.string("frontier://FrontierPrep/CMS_COND_GEOMETRY"),
label = cms.untracked.string("130503")
),

# Configurations
cms.PSet(record = cms.string("MagFieldConfigRcd"),
tag = cms.string("MagFieldConfig"),
connect = cms.untracked.string("sqlite_file:DB_Conf/MFConfig_71212_0T.db"),
label = cms.untracked.string("0T")
),
cms.PSet(record = cms.string("MagFieldConfigRcd"),
tag = cms.string("MagFieldConfig"),
connect = cms.untracked.string("sqlite_file:DB_Conf/MFConfig_71212_2T.db"),
label = cms.untracked.string("2T")
),
cms.PSet(record = cms.string("MagFieldConfigRcd"),
tag = cms.string("MagFieldConfig"),
connect = cms.untracked.string("sqlite_file:DB_Conf/MFConfig_71212_3T.db"),
label = cms.untracked.string("3T")
),
cms.PSet(record = cms.string("MagFieldConfigRcd"),
tag = cms.string("MagFieldConfig"),
connect = cms.untracked.string("sqlite_file:DB_Conf/MFConfig_71212_3_5T.db"),
label = cms.untracked.string("3.5T")
),
# cms.PSet(record = cms.string("MagFieldConfigRcd"), # Run 2, new version (130503)
# tag = cms.string("MagFieldConfig"),
# connect = cms.untracked.string("sqlite_file:DB_Conf/MFConfig_130503_Run2_3_5T.db"),
# label = cms.untracked.string("3.5T")
# ),
cms.PSet(record = cms.string("MagFieldConfigRcd"), #Run 1 default
tag = cms.string("MagFieldConfig"),
connect = cms.untracked.string("sqlite_file:DB_Conf/MFConfig_90322_2pi_scaled_3_8T.db"),
label = cms.untracked.string("3.8T")
),
# cms.PSet(record = cms.string("MagFieldConfigRcd"), #Run 2 default
# tag = cms.string("MagFieldConfig"),
# connect = cms.untracked.string("sqlite_file:DB_Conf/MFConfig_120812_Run2_3_8T.db"),
# label = cms.untracked.string("3.8T")
# ),
# cms.PSet(record = cms.string("MagFieldConfigRcd"), #Run 1, version 120812
# tag = cms.string("MagFieldConfig"),
# connect = cms.untracked.string("sqlite_file:DB_Conf/MFConfig_120812_Run1_3_8T.db"),
# label = cms.untracked.string("3.8T")
# ),
# cms.PSet(record = cms.string("MagFieldConfigRcd"), #Run 1, version 130503
# tag = cms.string("MagFieldConfig"),
# connect = cms.untracked.string("sqlite_file:DB_Conf/MFConfig_130503_Run1_3_8T.db"),
# label = cms.untracked.string("3.8T")
# ),
# cms.PSet(record = cms.string("MagFieldConfigRcd"), #Run 2, new version (130503)
# tag = cms.string("MagFieldConfig"),
# connect = cms.untracked.string("sqlite_file:DB_Conf/MFConfig_130503_Run2_3_8T.db"),
# label = cms.untracked.string("3.8T")
# ),
cms.PSet(record = cms.string("MagFieldConfigRcd"),
tag = cms.string("MagFieldConfig"),
connect = cms.untracked.string("sqlite_file:DB_Conf/MFConfig_71212_4T.db"),
label = cms.untracked.string("4T")
),


)

# process.VolumeBasedMagneticFieldESProducer.valueOverride = 17000

process.MessageLogger = cms.Service("MessageLogger",
categories = cms.untracked.vstring("MagneticField"),
destinations = cms.untracked.vstring("cout"),
cout = cms.untracked.PSet(
noLineBreaks = cms.untracked.bool(True),
threshold = cms.untracked.string("WARNING"),
WARNING = cms.untracked.PSet(
limit = cms.untracked.int32(0)
),
MagneticField = cms.untracked.PSet(
limit = cms.untracked.int32(10000000)
)
)
)

process.testField = cms.EDAnalyzer("testMagneticField")
process.p1 = cms.Path(process.testField)

9 changes: 4 additions & 5 deletions MagneticField/GeomBuilder/plugins/VolumeBasedMagneticFieldESProducerFromDB.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,17 +95,16 @@ std::auto_ptr<MagneticField> VolumeBasedMagneticFieldESProducerFromDB::produce(c
message = " (from valueOverride card)";
}
string configLabel = closerNominalLabel(current);
edm::LogInfo("MagneticField|AutoMagneticField") << "Current: " << current << message << "; using map configuration with label: " << configLabel;

// Get configuration
ESHandle<MagFieldConfig> confESH;
iRecord.getRecord<MagFieldConfigRcd>().get(configLabel, confESH);
const MagFieldConfig* conf = &*confESH;

if (debug) {
cout << "VolumeBasedMagneticFieldESProducerFromDB::produce() " << conf->version << endl;
}

edm::LogInfo("MagneticField|AutoMagneticField") << "Current: " << current << message << "; using map configuration with label: " << configLabel << endl
<< "Version: " << conf->version
<< " geometryVersion: " << conf->geometryVersion
<< " slaveFieldVersion: " << conf->slaveFieldVersion;
Copy link
Contributor

Choose a reason for hiding this comment

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

Very good (I assume you think LogInfo is the right level though you had "debug" initially)!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a part of the debug code mentioned above, so MessageLogger is appropriate in this specific case. Info is more appropriate IMO.


// Get the parametrized field
std::auto_ptr<MagneticField> paramField = ParametrizedMagneticFieldFactory::get(conf->slaveFieldVersion, conf->slaveFieldParameters);
Expand Down
29 changes: 25 additions & 4 deletions MagneticField/GeomBuilder/src/MagGeoBuilderFromDDD.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,13 +346,34 @@ void MagGeoBuilderFromDDD::build(const DDCompactView & cpva)

//Sort in phi
precomputed_value_sort(eVolumes.begin(), eVolumes.end(), ExtractPhi());

// Handle the -pi/pi boundary: volumes crossing it could be half at the begin and half at end of the sorted list.
// So, check if any of the volumes that should belong to the first bin (at -phi) are at the end of the list:
float lastBinPhi = phiClust.back();
handles::reverse_iterator ri = eVolumes.rbegin();
while ((*ri)->center().phi()>lastBinPhi) {++ri;}
if (ri!=eVolumes.rbegin()) {
// ri points to the first element that is within the last bin.
// We need to move the following element (ie ri.base()) to the beginning of the list,
handles::iterator newbeg = ri.base();
rotate(eVolumes.begin(),newbeg, eVolumes.end());
}

//Group volumes in sectors
int offset = eVolumes.size()/nESectors;
for (int i = 0; i<nESectors; ++i) {
int offset = eVolumes.size()/nESectors;
if (debug) cout << " Sector at phi = "
<< (*(eVolumes.begin()+((i)*offset)))->center().phi()
<< endl;
if (debug) {
cout << " Sector at phi = "
<< (*(eVolumes.begin()+((i)*offset)))->center().phi()
<< endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not new but cout is not good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I wrote, this is also something that activates much more checks than just printing a message (elsewhere in this class). There is no point in changing messages here because messages are issued only when running in debugging mode, and logDebug does not help here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Nicola,

If we were to test this in multi-threaded mode, cout may give garbage
output.
edm::LogPrint behaves visually exactly as cout (and is enabled by
default, because it's a verbatim equivalent of a LogWarning). A change
is rather trivial.

On 11/21/14, 6:46 AM, Nicola Amapane wrote:

In MagneticField/GeomBuilder/src/MagGeoBuilderFromDDD.cc:

for (int i = 0; i<nESectors; ++i) {

  • int offset = eVolumes.size()/nESectors;
  • if (debug) cout << " Sector at phi = "
  •        << (_(eVolumes.begin()+((i)_offset)))->center().phi()

  •        << endl;
  • if (debug) {
  •  cout << " Sector at phi = "

  •   << (_(eVolumes.begin()+((i)_offset)))->center().phi()

  •   << endl;

As I wrote, this is also something that activates much more checks than
just printing a message (elsewhere in this class). There is no point in
changing messages here because messages are issued only when running in
debugging mode, and logDebug does not help here.

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

Vyacheslav (Slava) Krutelyov
TAMU: Physics Dept Texas A&M MS4242, College Station, TX 77843-4242
CERN: 42-R-027
AIM/Skype: siava16 googleTalk: slava77@gmail.com
(630) 291-5128 Cell (US) +41 76 275 7116 Cell (CERN)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dear Slava,

In the 10 year lifetime of this code, nobody has been running it in
debug mode other than me. With all due respect, I really doubt you even
know how make a sense of what the debug mode would do.

When I'll need to test it in a multi-threaded mode, I will take care of
the needed modifications. Before that, it's just a waste of time.

Cheers,
Nicola

On 21-Nov-14 14:09, Slava Krutelyov wrote:

In MagneticField/GeomBuilder/src/MagGeoBuilderFromDDD.cc:

for (int i = 0; i<nESectors; ++i) {

  • int offset = eVolumes.size()/nESectors;
  • if (debug) cout << " Sector at phi = "
  •        << (_(eVolumes.begin()+((i)_offset)))->center().phi()

  •        << endl;
  • if (debug) {
  •  cout << " Sector at phi = "

  •   << (_(eVolumes.begin()+((i)_offset)))->center().phi()

  •   << endl;

Hi Nicola, If we were to test this in multi-threaded mode, cout may give
garbage output. edm::LogPrint behaves visually exactly as cout (and is
enabled by default, because it's a verbatim equivalent of a LogWarning).
A change is rather trivial.
… <#>
On 11/21/14, 6:46 AM, Nicola Amapane wrote: In
MagneticField/GeomBuilder/src/MagGeoBuilderFromDDD.cc: > for (int i = 0;
i<nESectors; ++i) { > - int offset = eVolumes.size()/nESectors; > - if
(debug) cout << " Sector at phi = " > - <<
(_(eVolumes.begin()+((i)offset)))->center().phi() > - << endl; > + if
(debug) { > + cout << " Sector at phi = " > + <<
((eVolumes.begin()+((i)_offset)))->center().phi() > + << endl; As I
wrote, this is also something that activates much more checks than just
printing a message (elsewhere in this class). There is no point in
changing messages here because messages are issued only when running in
debugging mode, and logDebug does not help here. — Reply to this email
directly or view it on GitHub

https://github.com/cms-sw/cmssw/pull/6514/files#r20712559.

Vyacheslav (Slava) Krutelyov TAMU: Physics Dept Texas A&M MS4242,
College Station, TX 77843-4242 CERN: 42-R-027 AIM/Skype: siava16
googleTalk: slava77@gmail.com (630) 291-5128 Cell (US) +41 76 275 7116

Cell (CERN)

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Nicola,

We are moving towards running multithreaded by default
and it's the first time since ever.
I tried to ward off some future issues, but whatever in this case

On 11/21/14, 7:14 AM, Nicola Amapane wrote:

In MagneticField/GeomBuilder/src/MagGeoBuilderFromDDD.cc:

for (int i = 0; i<nESectors; ++i) {

  • int offset = eVolumes.size()/nESectors;
  • if (debug) cout << " Sector at phi = "
  •        << (_(eVolumes.begin()+((i)_offset)))->center().phi()

  •        << endl;
  • if (debug) {
  •  cout << " Sector at phi = "

  •   << (_(eVolumes.begin()+((i)_offset)))->center().phi()

  •   << endl;

Dear Slava, In the 10 year lifetime of this code, nobody has been
running it in debug mode other than me. With all due respect, I really
doubt you even know how make a sense of what the debug mode would do.
When I'll need to test it in a multi-threaded mode, I will take care of
the needed modifications. Before that, it's just a waste of time.
Cheers, Nicola
… <#>
On 21-Nov-14 14:09, Slava Krutelyov wrote: In
MagneticField/GeomBuilder/src/MagGeoBuilderFromDDD.cc: > for (int i = 0;
i<nESectors; ++i) { > - int offset = eVolumes.size()/nESectors; > - if
(debug) cout << " Sector at phi = " > - <<
(_(eVolumes.begin()+((i)offset)))->center().phi() > - << endl; > + if
(debug) { > + cout << " Sector at phi = " > + <<
((eVolumes.begin()+((i)_offset)))->center().phi() > + << endl; Hi
Nicola, If we were to test this in multi-threaded mode, cout may give
garbage output. edm::LogPrint behaves visually exactly as cout (and is
enabled by default, because it's a verbatim equivalent of a LogWarning).
A change is rather trivial. … <#> On 11/21/14, 6:46 AM, Nicola Amapane
wrote: In MagneticField/GeomBuilder/src/MagGeoBuilderFromDDD.cc: > for
(int i = 0; i<nESectors; ++i) { > - int offset =
eVolumes.size()/nESectors; > - if (debug) cout << " Sector at phi = " >

  • << (_(eVolumes.begin()+((i)offset)))->center().phi() > - << endl; > +
    if (debug) { > + cout << " Sector at phi = " > + <<
    (    (eVolumes.begin()+((i)_offset)))->center().phi() > + << endl; As I
    wrote, this is also something that activates much more checks than just
    printing a message (elsewhere in this class). There is no point in
    changing messages here because messages are issued only when running in
    debugging mode, and logDebug does not help here. — Reply to this email
    directly or view it on GitHub
    https://github.com/cms-sw/cmssw/pull/6514/files#r20712559. --

    Vyacheslav (Slava) Krutelyov TAMU: Physics Dept Texas A&M MS4242,
    College Station, TX 77843-4242 CERN: 42-R-027 AIM/Skype: siava16
    googleTalk: slava77@gmail.com (630) 291-5128 Cell (US) +41 76 275 7116
    Cell (CERN)

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

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

Vyacheslav (Slava) Krutelyov
TAMU: Physics Dept Texas A&M MS4242, College Station, TX 77843-4242
CERN: 42-R-027
AIM/Skype: siava16 googleTalk: slava77@gmail.com
(630) 291-5128 Cell (US) +41 76 275 7116 Cell (CERN)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dear Slava,

Thanks for the warning. My point is that this specific code won't ever
be executed in debug mode again until I need to integrate the next
generation of maps, which won't be before 2016 at the very least least.
That day, I'll have to fix it indeed. Now, I have barely the time to
update code that IS run in HLT and offline. So, debug code that I will
maybe need to use again in two years from now is extremely low in any
scale of priority.

Cheers
Nicola

On 21-Nov-14 14:17, Slava Krutelyov wrote:

In MagneticField/GeomBuilder/src/MagGeoBuilderFromDDD.cc:

for (int i = 0; i<nESectors; ++i) {

  • int offset = eVolumes.size()/nESectors;
  • if (debug) cout << " Sector at phi = "
  •        << (_(eVolumes.begin()+((i)_offset)))->center().phi()

  •        << endl;
  • if (debug) {
  •  cout << " Sector at phi = "

  •   << (_(eVolumes.begin()+((i)_offset)))->center().phi()

  •   << endl;

Hi Nicola, We are moving towards running multithreaded by default and
it's the first time since ever. I tried to ward off some future issues,
but whatever in this case
… <#>
On 11/21/14, 7:14 AM, Nicola Amapane wrote: In
MagneticField/GeomBuilder/src/MagGeoBuilderFromDDD.cc: > for (int i = 0;
i<nESectors; ++i) { > - int offset = eVolumes.size()/nESectors; > - if
(debug) cout << " Sector at phi = " > - <<
(_(eVolumes.begin()+((i)offset)))->center().phi() > - << endl; > + if
(debug) { > + cout << " Sector at phi = " > + <<
((eVolumes.begin()+((i)offset)))->center().phi() > + << endl; Dear
Slava, In the 10 year lifetime of this code, nobody has been running it
in debug mode other than me. With all due respect, I really doubt you
even know how make a sense of what the debug mode would do. When I'll
need to test it in a multi-threaded mode, I will take care of the needed
modifications. Before that, it's just a waste of time. Cheers, Nicola …
<#> On 21-Nov-14 14:09, Slava Krutelyov wrote: In
MagneticField/GeomBuilder/src/MagGeoBuilderFromDDD.cc: > for (int i = 0;
i<nESectors; ++i) { > - int offset = eVolumes.size()/nESectors; > - if
(debug) cout << " Sector at phi = " > - <<
((eVolumes.begin()+((i)offset)))->center().phi() > - << endl; > + if
(debug) { > + cout << " Sector at phi = " > + <<
((eVolumes.begin()+((i)_offset)))->center().phi() > + << endl; Hi
Nicola, If we were to test this in multi-threaded mode, cout may give
garbage output. edm::LogPrint behaves visually exactly as cout (and is
enabled by default, because it's a verbatim equivalent of a LogWarning).
A change is rather trivial. … <#> On 11/21/14, 6:46 AM, Nicola Amapane
wrote: In MagneticField/GeomBuilder/src/MagGeoBuilderFromDDD.cc: > for
(int i = 0; i<nESectors; ++i) { > - int offset =
eVolumes.size()/nESectors; > - if (debug) cout << " Sector at phi = " >

  • << (_(eVolumes.begin()+((i)offset)))->center().phi() > - << endl; > +
    if (debug) { > + cout << " Sector at phi = " > + <<
    (    (eVolumes.begin()+((i)_offset)))->center().phi() > + << endl; As I
    wrote, this is also something that activates much more checks than just
    printing a message (elsewhere in this class). There is no point in
    changing messages here because messages are issued only when running in
    debugging mode, and logDebug does not help here. — Reply to this email
    directly or view it on GitHub
    https://github.com/cms-sw/cmssw/pull/6514/files#r20712559. --

    Vyacheslav (Slava) Krutelyov TAMU: Physics Dept Texas A&M MS4242,
    College Station, TX 77843-4242 CERN: 42-R-027 AIM/Skype: siava16
    googleTalk: slava77@gmail.com (630) 291-5128 Cell (US) +41 76 275 7116
    Cell (CERN)

    — Reply to this email directly or view it on GitHub
    https://github.com/cms-sw/cmssw/pull/6514/files#r20713625. — Reply to
    this email directly or view it on GitHub
    https://github.com/cms-sw/cmssw/pull/6514/files#r20713769.

    Vyacheslav (Slava) Krutelyov TAMU: Physics Dept Texas A&M MS4242,
    College Station, TX 77843-4242 CERN: 42-R-027 AIM/Skype: siava16
    googleTalk: slava77@gmail.com (630) 291-5128 Cell (US) +41 76 275 7116
    Cell (CERN)

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Dear Nicola,

Thanks for the clarification.
Maybe by 2016 we even get rid of couts centrally.

Cheers

On 11/21/14, 8:15 AM, Nicola Amapane wrote:

In MagneticField/GeomBuilder/src/MagGeoBuilderFromDDD.cc:

for (int i = 0; i<nESectors; ++i) {

  • int offset = eVolumes.size()/nESectors;
  • if (debug) cout << " Sector at phi = "
  •        << (_(eVolumes.begin()+((i)_offset)))->center().phi()

  •        << endl;
  • if (debug) {
  •  cout << " Sector at phi = "

  •   << (_(eVolumes.begin()+((i)_offset)))->center().phi()

  •   << endl;

Dear Slava, Thanks for the warning. My point is that this specific code
won't ever be executed in debug mode again until I need to integrate the
next generation of maps, which won't be before 2016 at the very least
least. That day, I'll have to fix it indeed. Now, I have barely the time
to update code that IS run in HLT and offline. So, debug code that I
will maybe need to use again in two years from now is extremely low in
any scale of priority. Cheers Nicola
… <#>
On 21-Nov-14 14:17, Slava Krutelyov wrote: In
MagneticField/GeomBuilder/src/MagGeoBuilderFromDDD.cc: > for (int i = 0;
i<nESectors; ++i) { > - int offset = eVolumes.size()/nESectors; > - if
(debug) cout << " Sector at phi = " > - <<
(_(eVolumes.begin()+((i)offset)))->center().phi() > - << endl; > + if
(debug) { > + cout << " Sector at phi = " > + <<
((eVolumes.begin()+((i)offset)))->center().phi() > + << endl; Hi
Nicola, We are moving towards running multithreaded by default and it's
the first time since ever. I tried to ward off some future issues, but
whatever in this case … <#> On 11/21/14, 7:14 AM, Nicola Amapane wrote:
In MagneticField/GeomBuilder/src/MagGeoBuilderFromDDD.cc: > for (int i =
0; i<nESectors; ++i) { > - int offset = eVolumes.size()/nESectors; > -
if (debug) cout << " Sector at phi = " > - <<
((eVolumes.begin()+((i)offset)))->center().phi() > - << endl; > + if
(debug) { > + cout << " Sector at phi = " > + <<
((eVolumes.begin()+((i)offset)))->center().phi() > + << endl; Dear
Slava, In the 10 year lifetime of this code, nobody has been running it
in debug mode other than me. With all due respect, I really doubt you
even know how make a sense of what the debug mode would do. When I'll
need to test it in a multi-threaded mode, I will take care of the needed
modifications. Before that, it's just a waste of time. Cheers, Nicola …
<#> On 21-Nov-14 14:09, Slava Krutelyov wrote: In
MagneticField/GeomBuilder/src/MagGeoBuilderFromDDD.cc: > for (int i = 0;
i<nESectors; ++i) { > - int offset = eVolumes.size()/nESectors; > - if
(debug) cout << " Sector at phi = " > - <<
((eVolumes.begin()+((i)offset)))->center().phi() > - << endl; > + if
(debug) { > + cout << " Sector at phi = " > + <<
((eVolumes.begin()+((i)_offset)))->center().phi() > + << endl; Hi
Nicola, If we were to test this in multi-threaded mode, cout may give
garbage output. edm::LogPrint behaves visually exactly as cout (and is
enabled by default, because it's a verbatim equivalent of a LogWarning).
A change is rather trivial. … <#> On 11/21/14, 6:46 AM, Nicola Amapane
wrote: In MagneticField/GeomBuilder/src/MagGeoBuilderFromDDD.cc: > for
(int i = 0; i<nESectors; ++i) { > - int offset =
eVolumes.size()/nESectors; > - if (debug) cout << " Sector at phi = " >

  • << (_(eVolumes.begin()+((i)offset)))->center().phi() > - << endl; > +
    if (debug) { > + cout << " Sector at phi = " > + <<
    (    (eVolumes.begin()+((i)_offset)))->center().phi() > + << endl; As I
    wrote, this is also something that activates much more checks than just
    printing a message (elsewhere in this class). There is no point in
    changing messages here because messages are issued only when running in
    debugging mode, and logDebug does not help here. — Reply to this email
    directly or view it on GitHub
    https://github.com/cms-sw/cmssw/pull/6514/files#r20712559. --

    Vyacheslav (Slava) Krutelyov TAMU: Physics Dept Texas A&M MS4242,
    College Station, TX 77843-4242 CERN: 42-R-027 AIM/Skype: siava16
    googleTalk: slava77@gmail.com (630) 291-5128 Cell (US) +41 76 275 7116
    Cell (CERN)

    — Reply to this email directly or view it on GitHub
    https://github.com/cms-sw/cmssw/pull/6514/files#r20713625. — Reply to
    this email directly or view it on GitHub
    https://github.com/cms-sw/cmssw/pull/6514/files#r20713769. --

    Vyacheslav (Slava) Krutelyov TAMU: Physics Dept Texas A&M MS4242,
    College Station, TX 77843-4242 CERN: 42-R-027 AIM/Skype: siava16
    googleTalk: slava77@gmail.com (630) 291-5128 Cell (US) +41 76 275 7116
    Cell (CERN)

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

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

Vyacheslav (Slava) Krutelyov
TAMU: Physics Dept Texas A&M MS4242, College Station, TX 77843-4242
CERN: 42-R-027
AIM/Skype: siava16 googleTalk: slava77@gmail.com
(630) 291-5128 Cell (US) +41 76 275 7116 Cell (CERN)

// Additional x-check: sectors are expected to be made by volumes with the same copyno
int secCopyNo = -1;
for (handles::const_iterator iv=eVolumes.begin()+((i)*offset); iv!=eVolumes.begin()+((i+1)*offset); ++iv){
if (secCopyNo>=0&& (*iv)->copyno!=secCopyNo) cout << "ERROR: volume copyno" << (*iv)->name << ":" << (*iv)->copyno << " differs from others in same sectors " << secCopyNo << endl;
secCopyNo = (*iv)->copyno;
}
}

sectors.push_back(eSector(eVolumes.begin()+((i)*offset),
eVolumes.begin()+((i+1)*offset)));
}
Expand Down
19 changes: 12 additions & 7 deletions MagneticField/GeomBuilder/src/volumeHandle.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,19 +461,24 @@ MagGeoBuilderFromDDD::volumeHandle::sides() const{
return result;
}

void MagGeoBuilderFromDDD::volumeHandle::printUniqueNames(handles::const_iterator begin, handles::const_iterator end ) {
void MagGeoBuilderFromDDD::volumeHandle::printUniqueNames(handles::const_iterator begin, handles::const_iterator end, bool uniq) {
std::vector<std::string> names;
for (handles::const_iterator i = begin;
i != end; ++i){
names.push_back((*i)->name);
if (uniq) names.push_back((*i)->name);
else names.push_back((*i)->name+":"+boost::lexical_cast<string>((*i)->copyno));
}

sort(names.begin(),names.end());
std::vector<std::string>::iterator i = unique(names.begin(),names.end());
int nvols = int(i - names.begin());
cout << nvols << " ";
copy(names.begin(), i, ostream_iterator<std::string>(cout, " "));

if (uniq) {
std::vector<std::string>::iterator i = unique(names.begin(),names.end());
int nvols = int(i - names.begin());
cout << nvols << " ";
copy(names.begin(), i, ostream_iterator<std::string>(cout, " "));
} else {
cout << names.size() << " ";
copy(names.begin(), names.end(), ostream_iterator<std::string>(cout, " "));
}
cout << endl;
}

Expand Down
2 changes: 1 addition & 1 deletion MagneticField/GeomBuilder/src/volumeHandle.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ class MagGeoBuilderFromDDD::volumeHandle {

/// Just for debugging...
static void printUniqueNames(handles::const_iterator begin,
handles::const_iterator end);
handles::const_iterator end, bool uniq=true);


// Phi ranges: Used by: LessDPhiMax; bSector; bSlab::[min|max]Phi();
Expand Down
6 changes: 6 additions & 0 deletions MagneticField/Layers/interface/MagVerbosity.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,15 @@
* \author N. Amapane - INFN Torino
*/

//#DEFINE MF_DEBUG

// Old debug control switch, being phased out
struct verbose {
#ifdef MF_DEBUG
static constexpr bool debugOut = true;
#else
static constexpr bool debugOut = false;
#endif
};

#endif
Expand Down