new CRV light default print #656

rlcee · 2021-11-11T17:51:20Z

After consulting with Ralf, reduce default Crv light file prints from several stanzas of

Reading CRV lookup tables /cvmfs/mu2e.opensciencegrid.org/DataFiles/CRVConditions/v6_0/LookupTable_4550_0 ... Done.
CRV sector 0 (R1) uses /cvmfs/mu2e.opensciencegrid.org/DataFiles/CRVConditions/v6_0/LookupTable_4550_0 with scintillation yield of 16548 photons/MeV

to
CRV light files: /cvmfs/mu2e.opensciencegrid.org/DataFiles/CRVConditions/v6_0
CRV light sectors: 23 hash:10539762652061218902
mirroring the default prints for other input files.

The previous format is still available with a debug flag.

In other default print cases, the hashes are over the entire input file content, but here the hash is just on the file names. Hashing the content would require intrusive code, or re-reading the files.

FNALbuild · 2021-11-11T17:51:58Z

Hi @rlcee,
You have proposed changes to files in these packages:

CRVResponse
Validation

which require these tests: build.

@Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main.

⌛ The following tests have been triggered for bcf92c0: build (Build queue has 2 jobs)

About FNALbuild. Code review on Mu2e/Offline.

FNALbuild · 2021-11-11T18:18:22Z

☀️ The tests passed at bcf92c0.

Test	Result	Details
merge	✅	Merged `bcf92c0` at `b826a22`
build (prof)	✅	Log file. Build time: 13 min 35 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
FIXME, TODO	〰️	TODO (1) FIXME (0) in 4 files
clang-tidy	〰️	0 errors 0 warnings

N.B. These results were obtained from a build of this Pull Request at bcf92c0 after being merged into the base branch at b826a22.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

kutschke · 2021-11-11T21:52:36Z

CRVResponse/src/CrvPhotonGenerator_module.cc

    }
+    std::cout << "CRV light sectors: " << _makeCrvPhotons.size() << " hash:" << hash <<std::endl;


The printout between lines 225 an 235 and on line 237 is not protected by a debug level. I am checking that this is intended.

Yes, intended. Recall when we discussed the print for the geometry and conditions files. We decided that they should not be able to be turned off, since we wanted to force the logging of the critical inputs, including the hash. This line follows that pattern.

ehrlich-uva · 2021-11-11T22:45:10Z

CRVResponse/src/CrvPhotonGenerator_module.cc

-      std::cout<<"CRV sector "<<i<<" ("<<_CRVSectors[i]<<") uses "<<_makeCrvPhotons.back()->GetFileName()<<" with scintillation yield of "<<_scintillationYields[i]<<" photons/MeV"<<std::endl;
+      if(_debug>0) std::cout<<"CRV sector "<<i<<" ("<<_CRVSectors[i]<<") uses "<<_makeCrvPhotons.back()->GetFileName()<<" with scintillation yield of "<<_scintillationYields[i]<<" photons/MeV"<<std::endl;


The sector-by-sector print outs of the scintillation yields is of some importance, because we are running simulations with different scintillation yields where the log files may be useful to recover the values that were used. What is the requirement to print out information at debug level 0?

My goal is to keep asking if prints are necessary and apply some pressure to keep it down. I'll leave it
to the group to determine "necessary". For this study, you could turn the debug to 1 in your fcl.
Or will the changes be made with different light files in different directories, in which
case the directory path print captures it? We also have the choice to set the debug level in
different ways, for example, leave it off by default but turn it on by default for all Production fcl.
We already do that for other flags. If the decision is to restore the print, we could try to reduce its
visual impact a bit.

@ehrlich-uva are you happy with Ray's reply or do you still want a change? If you are happy, please approve the PR.

The scintillation yields of the individual CRV sectors is important information. That's why I was wondering whether it could be kept in the log file regardless of the debug level.
As an alternative, we could set the default debug level to 1 for the CrvPhotonGenerator so that all relevant information gets printed out by default. In this case, debug>0 in MakeCrvPhotons.cc could be changed to debug>1, since this part is less relevant.
@rlcee The lookup-table files and directories don't change when different scintillation yields are used.

ehrlich-uva · 2021-11-15T01:24:18Z

CRVResponse/src/CrvPhotonGenerator_module.cc

@@ -202,18 +210,31 @@ namespace mu2e
        {
           tableLoaded=true;
           _makeCrvPhotons.emplace_back(_makeCrvPhotons[j]);
-           std::cout<<"CRV sector "<<i<<" ("<<_CRVSectors[i]<<") uses "<<_makeCrvPhotons.back()->GetFileName()<<std::endl;
+           if(_debug>0) std::cout<<"CRV sector "<<i<<" ("<<_CRVSectors[i]<<") uses "<<_makeCrvPhotons.back()->GetFileName()<<std::endl;


Could you please replace this line by
if(_debug>0) std::cout<<"CRV sector "<<i<<" ("<<_CRVSectors[i]<<") uses "<<_makeCrvPhotons.back()->GetFileName()<<" with scintillation yield of "<<_scintillationYields[i]<<" photons/MeV"<<std::endl;

ehrlich-uva · 2021-11-15T03:54:46Z

CRVResponse/src/CrvPhotonGenerator_module.cc

@@ -222,7 +222,6 @@ namespace mu2e
      filedirs.insert( std::filesystem::path(filespec).parent_path() );
      photonMaker->LoadLookupTable(filespec,_debug);
      photonMaker->SetScintillationYield(_scintillationYields[i]);
-      if(_debug>0) std::cout<<"CRV sector "<<i<<" ("<<_CRVSectors[i]<<") uses "<<_makeCrvPhotons.back()->GetFileName()<<" with scintillation yield of "<<_scintillationYields[i]<<" photons/MeV"<<std::endl;


This line should have stayed.

It looks like Ray has addressed this now. When you agree that this is ready, please approve the PR.

kutschke · 2021-11-15T20:15:43Z

@FNALbuild run build test

FNALbuild · 2021-11-15T20:17:19Z

⌛ The following tests have been triggered for 69ee3c8: build (Build queue has 1 jobs)

FNALbuild · 2021-11-15T20:45:52Z

☀️ The tests passed at 69ee3c8.

Test	Result	Details
merge	✅	Merged `69ee3c8` at `42f89e1`
build (prof)	✅	Log file. Build time: 15 min 00 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
FIXME, TODO	〰️	TODO (1) FIXME (0) in 4 files
clang-tidy	〰️	0 errors 0 warnings

N.B. These results were obtained from a build of this Pull Request at 69ee3c8 after being merged into the base branch at 42f89e1.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

FNALbuild · 2021-11-15T20:45:56Z

📝 The HEAD of main has changed to 6bfd2a1. Tests are now out of date.

ehrlich-uva · 2021-11-15T20:53:40Z

Approved.

kutschke · 2021-11-15T22:02:32Z

Thanks @ehrlich-uva . When I said, please approve, I meant to go to the "Files change" link at the top right of this page. In the upper right of that page, there is a green "Review changes" box. Click on that which will open a popup window. I there click the "Approve" radio button. If you have want to enter a comment you can do so. Then click on the green "Submit review" button. Please do so when you have a chance.

rlcee added 2 commits November 11, 2021 11:42

remove unnecessary endJob

c2bd49c

reduce default prints

bcf92c0

FNALbuild added the build running label Nov 11, 2021

FNALbuild added build finished and removed build running labels Nov 11, 2021

kutschke reviewed Nov 11, 2021

View reviewed changes

kutschke requested a review from ehrlich-uva November 11, 2021 21:53

kutschke self-assigned this Nov 11, 2021

ehrlich-uva reviewed Nov 11, 2021

View reviewed changes

ehrlich-uva reviewed Nov 15, 2021

View reviewed changes

combine prints

745ab65

FNALbuild added build pending and removed build finished labels Nov 15, 2021

ehrlich-uva reviewed Nov 15, 2021

View reviewed changes

correction

69ee3c8

FNALbuild added build running and removed build pending labels Nov 15, 2021

FNALbuild added build pending and removed build running labels Nov 15, 2021

ehrlich-uva approved these changes Nov 15, 2021

View reviewed changes

kutschke merged commit bea231a into Mu2e:main Nov 15, 2021

rlcee mentioned this pull request Nov 20, 2021

filter and CRV summary log text #640

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new CRV light default print #656

new CRV light default print #656

rlcee commented Nov 11, 2021

FNALbuild commented Nov 11, 2021

FNALbuild commented Nov 11, 2021

kutschke Nov 11, 2021

rlcee Nov 11, 2021

ehrlich-uva Nov 11, 2021

rlcee Nov 12, 2021

kutschke Nov 13, 2021

ehrlich-uva Nov 15, 2021

ehrlich-uva Nov 15, 2021 •

edited

Loading

ehrlich-uva Nov 15, 2021

kutschke Nov 15, 2021

kutschke commented Nov 15, 2021

FNALbuild commented Nov 15, 2021

FNALbuild commented Nov 15, 2021

FNALbuild commented Nov 15, 2021

ehrlich-uva commented Nov 15, 2021

kutschke commented Nov 15, 2021

		}
		std::cout << "CRV light sectors: " << _makeCrvPhotons.size() << " hash:" << hash <<std::endl;

		std::cout<<"CRV sector "<<i<<" ("<<_CRVSectors[i]<<") uses "<<_makeCrvPhotons.back()->GetFileName()<<" with scintillation yield of "<<_scintillationYields[i]<<" photons/MeV"<<std::endl;
		if(_debug>0) std::cout<<"CRV sector "<<i<<" ("<<_CRVSectors[i]<<") uses "<<_makeCrvPhotons.back()->GetFileName()<<" with scintillation yield of "<<_scintillationYields[i]<<" photons/MeV"<<std::endl;

new CRV light default print #656

new CRV light default print #656

Conversation

rlcee commented Nov 11, 2021

FNALbuild commented Nov 11, 2021

FNALbuild commented Nov 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehrlich-uva Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kutschke commented Nov 15, 2021

FNALbuild commented Nov 15, 2021

FNALbuild commented Nov 15, 2021

FNALbuild commented Nov 15, 2021

ehrlich-uva commented Nov 15, 2021

kutschke commented Nov 15, 2021

ehrlich-uva Nov 15, 2021 •

edited

Loading