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

Fix the issues causing FST codes not running #338

Merged
merged 1 commit into from
Apr 26, 2022
Merged

Fix the issues causing FST codes not running #338

merged 1 commit into from
Apr 26, 2022

Conversation

techuan-huang
Copy link
Contributor

@techuan-huang techuan-huang commented Apr 22, 2022

There are two issues in the current DEV causing FST codes not running.
The first one is StFstUtil not loaded before StFstRawHitMaker in the chain options. The second is the incorrect range of FST sensor id in StarDb geometry.

In order to run FST codes, I made following changes:

  1. Clean up duplicate chain options for FST.
  2. Add a chain option for StFstUtil.
  3. Change the range of sensor id from 1-108 to 1000-1107 in StarDb/Geometry/fst/fstSensorOnWedge.20211110.000001.C.

@@ -5,7 +5,7 @@ St_Survey *tableSet = new St_Survey("fstSensorOnWedge",108);
//
for(int i = 0; i < 108; i++){
memset(&row,0,tableSet->GetRowSize());
row.Id = i+1;
row.Id = i+1000;
Copy link
Member

Choose a reason for hiding this comment

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

Could you point to the code where it is used so we can see the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if somewhere else will be affected, but for what I tested, the problem is in StRoot/StFstHitMaker/StFstHitMaker.cxx:
The database is loaded with StFstDb and the table is assigned to a variable on line 44.
This variable is then used on line 192 for transforming the coordinates of the hit positions.
As defined on line 191, the sensorId should begin with 1000.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you, Techuan. I added Xu and Zhenyu here to make sure everyone is on the same page.
On a separate note, Xu and Zhenyu, as the PoC of FST are not listed as the code maintainers for StarDb/xxx/fst directory. I feel we may want to specify this, also for other subsystems, so the PRs can automatically pick up the reviewers.

Copy link
Member

Choose a reason for hiding this comment

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

the sensorId should begin with 1000

Ok, I see that in the corresponding database table these sensor ids start from 1000, so this macro is supposed to follow the same numbering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dmitri, thanks for checking.
Since there are already two reviewers approved, should we go ahead to merge this PR or we need to wait for your approval?

Copy link
Member

Choose a reason for hiding this comment

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

The formal requirements are met, so merge it anytime

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I will click the merge button.

Comment on lines -1591 to -1601

{"fstFastSim","fstChain","","StMcEvent,StEvent","StFstFastSimMaker","StFstSimMaker","FST fast simulator",
kFALSE},
{"fstRawHit", "", "fstChain", "fstUtil,fstDb","StFstRawHitMaker", "StFstRawHitMaker","FST raw hit maker",
kFALSE},
{"fstCluster", "", "fstChain", "fstRawHit","StFstClusterMaker", "StFstClusterMaker","FST Cluster maker",
kFALSE},
{"fstHit", "", "fstChain", "event,fstCluster", "StFstHitMaker", "StFstHitMaker","FST Hit Maker",
kFALSE},
{"fstUtil" , "", "fstChain", "", "", "StFstUtil", "Fst Utilities",
kFALSE},
Copy link
Member

Choose a reason for hiding this comment

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

Indeed these lines appear to be identical to the ones above them. These were approved twice in #266 and #282 about 3 and 2 months ago. So much for the huge PRs

Copy link
Contributor

@genevb genevb left a comment

Choose a reason for hiding this comment

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

I certainly approve of removing the duplication in BigFullChain.h.

Copy link
Contributor

@sunxuhit sunxuhit left a comment

Choose a reason for hiding this comment

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

These changes are already discussed in FST group and verified locally.

@techuan-huang techuan-huang merged commit b057f02 into star-bnl:main Apr 26, 2022
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

Successfully merging this pull request may close these issues.

5 participants