MF standardized regression tests + fix some loose ends in MF #28642
Conversation
|
The code-checks are being triggered in jenkins. |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28642/13197
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
The code-checks are being triggered in jenkins. |
|
SimPPS/PPSPixelDigiProducer/test/FromGun2DigiAnal_.py is indeed being removed also in #28190, I was not aware of that since it was not mentioned in the original discussion in #28280. I am not sure how to handle this, should I make a commit where I put back this file in the branch I used for this PR? Thanks, N. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28642/13201
|
|
A new Pull Request was created by @namapane (Nicola Amapane) for master. It involves the following packages: DetectorDescription/DDCMS @perrotta, @civanch, @Dr15Jones, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
+1 |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
| # Always use the standard sequence Configuration.StandardSequences.MagneticField_cff | ||
|
|
||
|
|
||
| DDDetectorESProducer = cms.ESSource("DDDetectorESProducer", |
There was a problem hiding this comment.
IIUC, this will not work in the standard setup due to a conflict with DDDetectorESProducer DD4hep_GeometrySim_cff.
perhaps add MF or smth for a different name?
There was a problem hiding this comment.
Good point, will do
There was a problem hiding this comment.
Funny, enough it does not work if I rename it to eg
process.DDDetectorMFESProducer = cms.ESSource("DDDetectorESProducer", ...
I then get:
An exception of category 'NoProxyException' occurred while
[0] Processing Event run: 1 lumi: 1 event: 1 stream: 0
[1] Running path 'p'
[2] Calling method for module testMagGeometryAnalyzer/'test'
[3] Using EventSetup component DD4hep_VolumeBasedMagneticFieldESProducer/'MagneticFieldESProducer' to make data MagneticField/'' in record Id
[4] Using EventSetup component DDCompactViewMFESProducer/'' to make data DDCompactView/'magfield' in record IdealMagneticFieldRecord
Exception Message:
No data of type "DDDetector" with label "magfield" in record "IdealMagneticFieldRecord"
It can be quickly reproduced editing
.@cvuosalo, @ianna, any idea of why this happens and how to avoid this?
@slava77 I will go on with the other fixes (in a new PR) while this is getting understood.
There was a problem hiding this comment.
I'm confused in what's failing and what is edited.
volumeBasedMagneticField_dd4hep_160812_cfi.py is currently included only in regression.py, but it runs only testMagneticField, while the error message is coming from testMagGeometryAnalyzer.
There was a problem hiding this comment.
I am trying to apply the modification in cmssw/MagneticField/GeomBuilder/test/python/testMagGeometry.py first, as mentioned in my message above. This contains a (stripped-down) copy of the same config, for the purpose of testing the geometry.
Of course I need to fix volumeBasedMagneticField_dd4hep_160812_cfi.py as well, but the problem would be the same.
| label = cms.untracked.string(''), | ||
| attribute = cms.string('magfield'), | ||
| value = cms.string('magfield'), | ||
| paramLabel = cms.string('parametrizedField'), |
There was a problem hiding this comment.
it looks like only DDDetector, attribute, and value are different from volumeBasedMagneticField_160812_cfi.
The ParametrizedMagneticFieldProducer above is also the same.
If this is supposed to stay for more than a week or so, it seems reasonable to refactor the common parts in volumeBasedMagneticField_160812_common_cfi
ParametrizedMagneticFieldProducer = cms.ESProducer("ParametrizedMagneticFieldProducer",
...
VolumeBasedMagneticFieldCommon = cms.PSet (
#common parts here
)There was a problem hiding this comment.
It is supposed to stay until DD4Hep becomes the default and DDD is dropped, which I guess is not happening in a week or so.
I have a slight preference for keeping the minimum number of cfi files since they quickly get confusing (one more is to be added for the DD4Hep producer from DB), but OK if you insist...
BTW could you suggest what I should do with the file that I removed (FromGun2DigiAnal_.py) which is also being addressed in another PR (see above)? Thanks.
I didn't look inside: does this file require modifications to be consistent with this PR if it is not removed? If no, I have less strong opinion and can leave it to a coin-toss for removing here and possible rebase need in #28190 or dropping its removal here . |
|
It just need to be removed (nothing else affected), which I did here since I did not hear about anything happening after the discussion in #28280. I think it belongs better to #28190, so given it is removed there I would drop the removal here. My question is just how to do this in git, should I just push a commit where I re-add back the original file, or should I use some arcane git spell? |
it's better to rebase interactively and remove the commit that removed the file. |
|
Sorry but I have no idea no of how to do this practically. Can you send me more detailed instructions? Otherwise I can close this PR and make a new one... |
I see that you removed the file in a commit that includes other changes. |
|
Replaced by #28665. |
PR description:
PR validation:
Ran regression tests against earlier regression files (produced in past CMSSW releases) for all possible combinations of era, current, and MF producer type.