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

FilledBunches and CollidingBunches removed from the makeScanFileII.py #35

Open
oturkot opened this issue Jul 31, 2017 · 15 comments
Open
Assignees

Comments

@oturkot
Copy link

oturkot commented Jul 31, 2017

@anbabaev

@capalmer85 @karacheb

In the commit 58cd981 FilledBunches and CollidingBunches were commented out from the makeScanFileII.py, and after that removed.
But these information is still required, e.g by
dataPrepII_PCC/makePCCRateFile.py
makeBeamCurrentFileII.py

Was it done intentionally ? If yes, what to do with scripts which require these information from the Scan_****.pkl file ?

@anbabaev
Copy link
Contributor

Hi.

makeBeamCurrentFileII.py does not reqiure information on bunches from Scan_****.pkl, if you use the version from the master branch (I've checked this just now)
Concerning makePCCRateFile.py - I think, you should change the line:
from inputDataReader import *
to the new version of inputDataReader:
from inputDataReaderII import *

@oturkot
Copy link
Author

oturkot commented Aug 3, 2017

@anbabaev

Dear Anton,

Thank you for your fast reply, I have a copy of master branch from June 20, 2017 (there were no updates after that yet), and I am getting an error:
File "/afs/cern.ch/work/o/oturkot/Lumi/VdMFramework/makeBeamCurrentFileII.py", line 321, in doMakeBeamCurrentFile
CollidingBunches = scanInfo["CollidingBunches"]

I have just checked the "makeBeamCurrentFileII.py" from the master branch:
https://github.com/CMS-LUMI-POG/VdMFramework/blob/master/makeBeamCurrentFileII.py
and it also has "CollidingBunches = scanInfo["CollidingBunches"]" in line 321.

As for the "makePCCRateFile.py" - thank you, import of "inputDataReaderII.py" helped to avoid previous error, but there is a new one:
Problem in makePCCRateFile_config.json, specified PCC_BCID not among colliding bunches specified in scan file, exit program
which is line 213 in:
https://github.com/CMS-LUMI-POG/VdMFramework/blob/master/dataPrepII_PCC/makePCCRateFile.py

Please let me know if you have some more ideas,
Thank you,
Oleksii.

@capalmer85
Copy link
Contributor

capalmer85 commented Aug 3, 2017 via email

@anbabaev
Copy link
Contributor

anbabaev commented Aug 4, 2017

Hi

Ah, Indeed. Somebody added function getCurrentsFromTimber and did not check that the Framework works after this. We uses the version before the function getCurrentsFromTimber has been added.
Do you work with Timber data?

@capalmer85
Copy link
Contributor

@gkrintir Can you help debug this since you added the Timber functionality? We need a patch for this ASAP.

@gkrintir
Copy link
Contributor

gkrintir commented Aug 4, 2017

Hello,
I am bit confused what the problem is because I explicitly checked that. The getCurrentsFromTimber function doesn't intervene with filledbunches at all, so they are left untouched as they are in the scan file.

@anbabaev can you paste which configuration are you using ?

@anbabaev
Copy link
Contributor

anbabaev commented Aug 4, 2017

Hi.
@gkrintir As far as I understand the problem is:

  1. In the 'oldest' Framework' the list of colliding bunches were formed in makeScanFileII.py based on the first scan point of the first scan.
  2. We had some problems with this feature of the the 'oldest' Framework. Few monthes ago we changed the way how the list of colliding bunches is formed. Now it is defined at each scan point separately. Correspondingly, all links to colliding (and filled) bunches data were removed from all files of the Framework. Simultaneously, fields "CollidingBunches", "FilledBunchesB1", "FilledBunchesB2" (and the corresponding code in makeScanFileII.py) were extra and have been removed from scanInfo. This is our work version now.
  3. When you added Timber functionality you also added the code on the colliding bunches from the 'oldest' Framework (lines 321-323 of makeBeamCurrentII.py). But scanInfo does not contain these fields anymore! This is why @oturkot has the mistake.

@anbabaev
Copy link
Contributor

anbabaev commented Aug 4, 2017

@oturkot

  1. If you do not work with Timber you can comment lines 321-323:
    CollidingBunches = scanInfo["CollidingBunches"]
    FilledBunchesB1 = scanInfo["FilledBunchesB1"]
    FilledBunchesB2 = scanInfo["FilledBunchesB2"]
    I think, this is the fastest way.
  2. If you need Timber, I think you can use old version of makeScanFileII.py. I think, somebody has this file.
  3. The right way would be to obtain colliding and filled bunches in getCurrentsFromTimber separately at each scanpoint. But I do not now if it is possible.

@gkrintir
Copy link
Contributor

gkrintir commented Aug 4, 2017

Ah ok I see, thx. I missed this because I didn't generate one scan file from scratch.

Yes, please comment out these lines and don't use the Timber function for the moment.

But I don't see the reason why the info about bunches were REMOVED from the scan file. This completely breaks the flow with previous configurations. The proper way is to define a separate way of reading the bunches per scan point, if user wishes. And this should be configurable and not hard coded. @capalmer85 am I missing smth here ?

@anbabaev
Copy link
Contributor

anbabaev commented Aug 4, 2017

Do not worry. The 'new' version is fully compatible with old fills. I've tested the code with 2015-2016 data starting from fill 4266.
Of course, one can return this operations in makeScanFileII.py but it is not used neither for actual fills nor for old ones.
TR_Babaev_vdM_Framework.pdf

@gkrintir
Copy link
Contributor

gkrintir commented Aug 4, 2017

Yes, that's a very nice and meticulous kind of work. However, imagine that I want to reproduce an old result (buggy or not) so as to compare it with all the fixs you suggest. With this commit it gets impossible, right ?

Also, do we know why this happens ? It's worrisome information that should be available per fill is not available for all scans. This points to problems with DIP protocol.

I think that the framework should work for some time with both configurations i.e, "old" (before your commit) spitting out the warnings and "new" (after your commit). We should understand whether the results are different, how&why, and only then fully migrate to "new". Was any validation test performed before the migration, e.g. compare x-section results ? If so which fill(s) ?

@anbabaev
Copy link
Contributor

anbabaev commented Aug 5, 2017

All these have been discussed earlier. I am not a github moderator and can not change the master branch by my own will :)

  1. This happens because FBCT is too low and the current is not filled in hd5 (following Anne's explanation).
  2. The 'old' version can not work when filled BCID lists are different at scan points.
  3. The 'new' version gives the identical results as the 'old' version. Tests were carried out with 4266, 4954, 5020. You can make additional tests if you have doubts.
  4. If BCID is not filled, the information on missed scanpoints is written in logs and is shown on screen.

@gkrintir
Copy link
Contributor

gkrintir commented Aug 5, 2017

Thanks Anton, that's re assuring then.

Yes (1) e.g. happened during pPb 2016 period due to FBCT failure. That's why we wrote the Timber function to read per bunch values from BQM.

Ok, what I suggest then is to simply put back to makeScanFileII.py the info about FilledBunches and CollidingBunches. This should be anyway independent from the way we later fill the list of bunches. I ll prepare the commit.

@oturkot
Copy link
Author

oturkot commented Aug 6, 2017

Dear All,

Thank you a lot for fast and useful replies, after commenting out lines 321-323 the "makeBeamCurrentFileII.py" works. I have checked the workflow for PCC luminometer without corrections, and it looks like that only "dataPrepII_PCC/makePCCRateFile.py" needs to be updated to work with new implementations (I assume makeRate scripts for other luminometers might need similar updates). At the moment the rate files stored in the master branch can be used.

@gkrintir
Copy link
Contributor

gkrintir commented Aug 6, 2017

Hi all,

simply putting the info about bunches in scan file: #36

solves the issue for both Timber and dataPrepII_PCC/makePCCRateFile.py.

As a reminder (as stated in README), if you want to enable the Timber function you have to specify it in the makeBeamCurrentFileConfig fragment by "ReadFromTimber" : true option.

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

No branches or pull requests

5 participants