add the RHICf library (for only storing raw data in BFC level)

[ggfdsa10]

1. new 2 makers a) StRHICfDbMaker (RHICf calibration data table maker) b) StRHICfRawHitMaker (storing raw data maker)

2. modified StEvent, StBFChain and add some new RHICf class The revised classes are essential for makers to add later. (RHICf calibration and reconstruction makers)

3. new MuRHICf class (for store in MuDst) and some MuDst base class modifying

4. add the StRHICfUtil folder and class (there to be added another reconstruction tool)

[starsdong]

Hi Seunghwan,

Thank you for putting this PR up. Just to clarify how we proceed with the review.

1) These requested updates will go through the designated reviews BFChain: Gene (@genevb) StEvent: Jason (@klendathu2k) MuDstMaker: Daniel B. (@jdbrice)

2) StRHICfDbMaker and StRHICfRawHitMaker are two new makers. We should go through the standard code review process. I will ask a couple of our colleagues to help on this (follow up on emails)

3) StRHICfUtil directory I am wondering whether you need this for production (meaning daq->MuDst) or they are used at the analyses level?

Thanks

[ggfdsa10]

Hello Xin,

Thank you for your follow-up. Mostly, StRHICfUtil works at the analysis level (calibration and reconstruction). But, There is some function (such as the tower size checker in StRHICfFunction in StRHICfUtil folder). This tiny function (this name is checkGSOBarSize() ) is used in many parts such as StRHICfRawHitMaker, StMuRHICfUtil, etc.

so I would like to add this directory.

Thank you.

P.S: Also, StRHICfFunction includes the RHICf run type checker. this function is very important for reconstruction.

[starsdong]

Thanks to Dmitry A. and Jeff for help review the new makers (StRHICfDbMaker and StRHICfRawHitMaker). Dmitry and Jeff are added to the reviewer list.

[ggfdsa10]

I think these PR test errors are caused by StRHICfFunction class not being recognized in StMuDST. should I separate the function like a checkGSOBarSize() from the BFC level? If I should do that, I will revise the StRHICfUtil directory is used only for the analysis level (MuDst -> picoDst)

[plexoos]

Seunghwan, for test purposes, can you provide commands (and input files) to convert raw and/or simulated data to StEven and muDst formats? Thanks

[ggfdsa10]

Hello Dmitry, I'm attached runBfc.C file link. https://github.com/ggfdsa10/STAR-RHICf/blob/main/runBfc.C

"rhicfdat" BFC option is RHICf raw data saving option for convert from .daq to MuDst. and that code includes one .daq file path. "st_rhicf_18178002_raw_1000004.daq"

Thank you.

[ggfdsa10]

Dear All, I think all of the PR test actions are checkers about the process of BFC -> MuDst level.

As I said before, that error seems caused by StRHICfFunction class in StMuRHICfUtil class. also, all of that actions do not include "RHICfUtil" lib that is "rhicfutil" BFC option. and that option is used by only RHICf process (that library is generated from the StRHICfUtil directory)

due to people who are not using RHICf do not need to call the RHICfUtil library, separating between StMuDST and StRHICfUtil is necessary

For this reason, I have removed the StRHICfFunction class header and functions at StMuRHICfUtil class. But, In the "StRHICfRawHitMaker", exist the StRHICfFunction yet.

I'm not sure if I can upload the Pull Request again myself. but I would like to request the revised StMuRHICfUtil class again. I'm so sorry if you already reviewed this part.

Thank you

[jml985]

It appears that I was removed as a reviewer (which is fine with me). I don't have any opinions or insight into all the failed CE tests.

I did however take a look at the StRHICfDbMaker and StRHICfHitMaker additions. I think the Db Maker probably meets the normal standards just fine. The tables are filled once at the beginning of each run, and if there were more than one run would be memory leaks, but I think this is impossible for the offline chain. The function for deleting tables is empty. The only code is the setup in the initRun() and a bunch of helper functions translating the DB data into detector specific items. I don't see any structural issues there.

In the stRHICfHitMaker codes, the setup logic in lines 56-79 is very complex and the actual logic is very simple. The actual logic appears to be a simple linear assignment of variables each dependent on previous step:

if(!eventPtr) return kStFatal;

mRHICfCollection = eventPtr->rhicfCollection(); if(!mRHICfCollection) mRHICfCollection=new StRHICfCollection; if(!mRHICfCollection) return kStFatal;

mRHICfRawHitColl=mRHICfCollection->rawHitCollection(); if(!mRHICfRawHitColl) return kStFatal;

But the existing is logically unclear, and the setupRHICfCollection() function does nothing except make it less clear.

[starsdong]

Hi Jeff, I guess you were probably accidentally removed. I added you back in. For everyone, Dmitry A. and Jeff L. were asked and agreed to review the new two makers: StRHICfDbMaker and StRHICfRawHitMaker as our normal review procedure for new codes.

[ggfdsa10]

I'm so sorry for my mistake, I don't know why I removed a reviewer. I guess there is something wrong when I clicked the re-request button

anyway, I don't know am I understanding your comments. but, as far as I understood your comments, StRHICfRawHitMaker needs to simplify a code. such as your comments:

**if(!eventPtr) return kStFatal;

mRHICfCollection = eventPtr->rhicfCollection(); if(!mRHICfCollection) mRHICfCollection=new StRHICfCollection; if(!mRHICfCollection) return kStFatal;

mRHICfRawHitColl=mRHICfCollection->rawHitCollection(); if(!mRHICfRawHitColl) return kStFatal;**

Also, that have to get to need the eventPtr->setRHICfCollection(mRHICfCollection) because there is no RHICfCollection at all of the event starting points. I've already checked when that code is present or not present.

and then, as per your other comment, StRHICfDbMaker is well done but should remove unnecessary functions like the deleteArrays(). If I was wrong, please let me know.

Thank you!

[ggfdsa10]

oh... I'm very sorry for Jason. when I clicked the re-requested button, removed a reviewer again. :( I will NOT click this button.

[starsdong]

Hi All, The two new makers are reviewed and approved by reviewers. We need to follow up on other changes in this PR too. @klendathu2k, on the StEvent containers, do you have any further comments/suggestions? @genevb, any request on the BFChain options? @jdbrice, could you please take a look at the MuDst changes? THanks and Best Regards

[klendathu2k]

Hi Xin,

I see many of my comments resolved. But there are some memory allocations that need to be cleaned up.

Cheers, Jason

On 2023-01-05 13:11, Xin Dong wrote:

Hi All, The two new makers are reviewed and approved by reviewers. We need to follow up on other changes in this PR too. @klendathu2k [1], on the StEvent containers, do you have any further comments/suggestions? @genevb [2], any request on the BFChain options? @jdbrice [3], could you please take a look at the MuDst changes? THanks and Best Regards

-- Reply to this email directly, view it on GitHub [4], or unsubscribe [5]. You are receiving this because you were mentioned.Message ID: @.***>

Links:

[1] https://github.com/klendathu2k [2] https://github.com/genevb [3] https://github.com/jdbrice [4] https://github.com/star-bnl/star-sw/pull/460#issuecomment-1372562548 [5] https://github.com/notifications/unsubscribe-auth/ANL4LVEAZX5NZDV3D44N3VDWQ4FGLANCNFSM6AAAAAASYHPUV4

[plexoos]

I'm attached runBfc.C file link. https://github.com/ggfdsa10/STAR-RHICf/blob/main/runBfc.C

"rhicfdat" BFC option is RHICf raw data saving option for convert from .daq to MuDst. and that code includes one .daq file path. "st_rhicf_18178002_raw_1000004.daq"

The daq file and the chain options have been added to CI tests. See #476 I would recommend to incorporate this along with modifications to StEvent and StRHICfRawHitMaker because that's what rhicfdat option depends on.

[ggfdsa10]

Hello Jason, Please tell me if there is anything I need to revise the code to arrange the parts of the allocation memory. Thank you

[ggfdsa10]

Hello Dmitri, I don't understand your comments. Do you mean, I just need to add this macro https://github.com/ggfdsa10/STAR-RHICf/blob/main/runBfc.C in this pull request?

If I don't need to additional revise some BFC options, I will solve Gene's request as below.

Gene: "I don't see codes for StRHICfHitMaker and StRHICfPointMaker. Am I missing something? Are those coming later? If so, I think the chain options should be introduced when those are introduced."

Please let me know if I'm wrong. Thank you

[plexoos]

I don't understand your comments. Do you mean, I just need to add this macro https://github.com/ggfdsa10/STAR-RHICf/blob/main/runBfc.C in this pull request?

No, you don't need to add your macro. In order to enable that test, you should add the changes from #476 (for example, by cherry-picking the commit) onto this PR branch. If you don't know how to do this, let me know and I'll do it.

Your test, as it is now, will simply make calls to StRHICfRawHitMaker and hopefully fill StEvent with RHICf data. If, however, you have a different macro that somehow verifies the content of the produced output files, I'd be happy to help you to accommodate it as a CI test.

If I don't need to additional revise some BFC options, I will solve Gene's request as below.

Gene: "I don't see codes for StRHICfHitMaker and StRHICfPointMaker. Am I missing something? Are those coming later? If so, I think the chain options should be introduced when those are introduced."

Yes, I agree with Gene, the options calling the non-existent makers should be removed from this PR and introduced along with those new makers in a separate PR. I would add that introducing tests for the new options is highly desirable.

[ggfdsa10]

Dear Dmitri,

Now I'm clear about what are you saying. I don't know how to add a CI test to this request. If you help me with this part I appreciate you.

Also, I share some macro file that is verifying for output MuDst file with you. https://github.com/ggfdsa10/STAR-RHICf/blob/main/openMuDst.C

If this macro is something wrong or does not function, please tell me Also, if there's anything I need to do, please let me know anytime.

Thank you!

[plexoos]

Hi Seunghwan,

I pushed the change enabling the CI test to this PR branch. Please note that your code does not compile due to the following errors:

#10 2398.2 g++ -m64 -fPIC -pipe -Wall -Woverloaded-virtual -std=c++0x -Wno-long-long -pthread -Wno-deprecated-declarations -Werror -O2 -g -falign-loops -falign-jumps -falign-functions -Dsl79_gcc485 -D__ROOT__ -DNEW_DAQ_READER -I. -IStRoot/StRHICfRawHitMaker -IStRoot/RTS/src -IStRoot/RTS/include -IStRoot/RTS/trg/include -I. -IStRoot -I.sl79_gcc485/include -I/opt/software/linux-scientific7-x86_64/gcc-4.8.5/root-5.34.38-l3v6vso6qgojm4l2ctwjojs6trbt4hpn/include -c .sl79_gcc485/OBJ/StRoot/StRHICfRawHitMaker/StRHICfRawHitMaker.cxx -o .sl79_gcc485/OBJ/StRoot/StRHICfRawHitMaker/StRHICfRawHitMaker.o
#10 2399.1 .sl79_gcc485/OBJ/StRoot/StRHICfRawHitMaker/StRHICfRawHitMaker.cxx: In member function 'virtual Int_t StRHICfRawHitMaker::Make()':
#10 2399.1 .sl79_gcc485/OBJ/StRoot/StRHICfRawHitMaker/StRHICfRawHitMaker.cxx:70:23: error: 'class StRHICfCollection' has no member named 'setRunNumber'
#10 2399.1    mRHICfCollection -> setRunNumber(gendata[0]);
#10 2399.1                        ^
#10 2399.1 .sl79_gcc485/OBJ/StRoot/StRHICfRawHitMaker/StRHICfRawHitMaker.cxx:71:23: error: 'class StRHICfCollection' has no member named 'setEventNumber'
#10 2399.1    mRHICfCollection -> setEventNumber(gendata[1]);
#10 2399.1                        ^

[ggfdsa10]

Ok that is revised Thank you

[ggfdsa10]

Dear All,

I was wondering if you could tell me when we can receive the code review approval and merge this request if you get a chance. After this review, We would like to produce all of the raw data to muDst. Please let me know if there is anything I need to do to review.

Thank you for your convenience, Seunghwan

[ggfdsa10]

Hello Dmitri,

Thank you for your comments and sorry for I didn't follow the STAR guidelines. I think all of the files in this PR have to revise part of the indentation. Also, In my PR files, Is there any other STAR coding guideline that I'm missing? I would appreciate it if you could let me know.

Seunghwan

[ggfdsa10]

Dear All,

I'm sorry for being late. now I modified the indentation of all code and n-dimension data arrays in StRHICfHit, StMuRHICfHit according to comments.

These commits were designed more can be reduced memory. also, when someone wants detailed n-dimension data in StRHICfHit, he can obtain that immediately.

Please review these codes and the remaining codes if you get a chance.

Seunghwan

[plexoos]

If there are no further comments, is it okay to merge this PR?

[ggfdsa10]

Thank you for merging! However, there seems to be a problem with the CI test docker. Please let me know if there is anything I can do to fix this problem.

Sincerely, Seunghwan

[plexoos]

Seunghwan, there was a problem with GitHub package repository access. They seem have fixed the problem and I restarted the jobs

[ggfdsa10]

Dear All,

New change is commit for solve the last change requested one. I'm so sorry for the inconvenience caused by committing after the commit request.

Seunghwan.

[plexoos]

Seunghwan, just create a new PR with fixes. Thanks

[ggfdsa10]

Dear All,

I created a new pull request for Dmitry's request. #511 I'm very sorry for the inconvenience.

Seunghwan

[plexoos]

Oh, so this one was not really merged... Ok, seems like another chance to review the code in #511