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

GMT #680

Merged
merged 49 commits into from
Feb 7, 2025
Merged

GMT #680

merged 49 commits into from
Feb 7, 2025

Conversation

nigmatkulov
Copy link
Member

@nigmatkulov nigmatkulov commented Apr 10, 2024

Commit GMT-related classes and macroses. Also, update StarDb/geometry for GMT with the latest alignment.

Copy link
Contributor

@klendathu2k klendathu2k left a comment

Choose a reason for hiding this comment

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

Few suggestions indicated. Few changes should be made,... StGmtAligner should be a maker, and prefer c++ native types (e.g. int) over ROOT types (Int_t) when a class is not intended for IO.

StRoot/StAnalysisMaker/StAnalysisMaker.cxx Outdated Show resolved Hide resolved
StRoot/StEvent/StGmtHit.h Outdated Show resolved Hide resolved
StRoot/StEvent/StGmtPoint.cxx Outdated Show resolved Hide resolved
StRoot/StEvent/StGmtStrip.h Outdated Show resolved Hide resolved
StRoot/StEvent/StGmtStripCollection.h Outdated Show resolved Hide resolved
StRoot/StEvent/StGmtStripCollection.h Outdated Show resolved Hide resolved
StRoot/StEvent/StGmtStripCollection.h Outdated Show resolved Hide resolved
StRoot/StGmtAligner/StGmtAligner.cxx Outdated Show resolved Hide resolved
StRoot/StGmtAligner/runGmtTree.C Outdated Show resolved Hide resolved
StRoot/StGmtClusterMaker/StGmtClusterMaker.cxx Outdated Show resolved Hide resolved
Copy link
Member

@plexoos plexoos left a comment

Choose a reason for hiding this comment

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

Before I do another round of comments on this massive PR, I would like to see the compiler errors addressed.

StRoot/StEvent/StGmtHit.h Outdated Show resolved Hide resolved
StRoot/StEvent/StGmtHitCollection.cxx Outdated Show resolved Hide resolved
StRoot/StEvent/StGmtPoint.cxx Outdated Show resolved Hide resolved
StRoot/StEvent/StGmtStrip.cxx Outdated Show resolved Hide resolved
StRoot/StEvent/StGmtStripCollection.cxx Outdated Show resolved Hide resolved
StRoot/StGmtAligmentMaker/EventT.h Outdated Show resolved Hide resolved
StRoot/StGmtAligmentMaker/HitT.h Outdated Show resolved Hide resolved
StRoot/StGmtAligmentMaker/StGmtAligner.cxx Outdated Show resolved Hide resolved
@fgeurts
Copy link
Member

fgeurts commented Aug 21, 2024

Hi, what is the status of this PR? Have all comments been addressed? If so, can we wrap this one up and prepare this to be merged into the STAR library?

@plexoos
Copy link
Member

plexoos commented Jan 22, 2025

hm... that's interesting:

#10 1435.1 .sl79_gcc485/OBJ/StRoot/StGmtClusterMaker/StGmtClusterMaker.cxx: In member function 'TF1* StGmtClusterMaker::FindPeaks(TH1F*)':
#10 1435.1 .sl79_gcc485/OBJ/StRoot/StGmtClusterMaker/StGmtClusterMaker.cxx:47:38: error: cannot convert 'Double_t* {aka double*}' to 'float*' in initialization
#10 1435.1    float* xpeaks = spect.GetPositionX();

@nigmatkulov
Copy link
Member Author

hm... that's interesting:

#10 1435.1 .sl79_gcc485/OBJ/StRoot/StGmtClusterMaker/StGmtClusterMaker.cxx: In member function 'TF1* StGmtClusterMaker::FindPeaks(TH1F*)':
#10 1435.1 .sl79_gcc485/OBJ/StRoot/StGmtClusterMaker/StGmtClusterMaker.cxx:47:38: error: cannot convert 'Double_t* {aka double*}' to 'float*' in initialization
#10 1435.1    float* xpeaks = spect.GetPositionX();

The reason is that ROOT5 and ROOT6 use different data types for GetPositionX. I'm trying to overpass it with the auto keyword. Let's see if it will work

@nigmatkulov
Copy link
Member Author

nigmatkulov commented Jan 23, 2025

@plexoos , please take a look at the actions. The current errors are due to the versioning of actions:

Error: This request automatically failed because it uses a deprecated version of actions/upload-artifact: v3. Learn more: https://github.blog/changelog/2024-04-16-deprecation-notice-v3-of-the-artifact-actions/

@plexoos
Copy link
Member

plexoos commented Jan 23, 2025

GitHub is experiencing issues with actions at the moment. You can check the status at https://www.githubstatus.com/
Screenshot 2025-01-23 at 10 20 45 AM

@nigmatkulov
Copy link
Member Author

nigmatkulov commented Jan 23, 2025

It seems to be fixed by now, but the error persists... I restarted jobs several times with the same result.
image

@plexoos
Copy link
Member

plexoos commented Jan 23, 2025

#727 should fix this

StRoot/StGmtAlignmentMaker/TrackT.h Outdated Show resolved Hide resolved
StRoot/StEvent/StGmtStrip.cxx Outdated Show resolved Hide resolved
StRoot/StGmtAlignmentMaker/EventT.h Outdated Show resolved Hide resolved
StRoot/StGmtAlignmentMaker/StGmtAlignmentMaker.h Outdated Show resolved Hide resolved
StRoot/StGmtClusterMaker/StGmtClusterMaker.cxx Outdated Show resolved Hide resolved
StRoot/StGmtAlignmentMaker/EventT.cxx Outdated Show resolved Hide resolved
StRoot/StGmtAlignmentMaker/EventT.cxx Outdated Show resolved Hide resolved
StRoot/StGmtAlignmentMaker/EventT.cxx Outdated Show resolved Hide resolved
Comment on lines +200 to +219
if (Debug()>3) {
canv->Divide(2,2);

canv->cd(1);
profX[module]->Draw();
canv->cd(2);
profY[module]->Draw();

canv->cd(3);
histX->Draw();
canv->cd(4);
histY->Draw();

canv->Modified();
canv->Update();
canv->Draw();
if (nClusX || nClusY) {
while (!gSystem->ProcessEvents()){gSystem->Sleep(200);}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

So, if someone wants to debug a different maker by running a BFC chain with GMT options, they’ll have to suffer because you decided to draw histograms and introduce artificial delays...

StRoot/StGmtRawMaker/StGmtRawMaker.h Outdated Show resolved Hide resolved
@plexoos plexoos merged commit 39e4e4f into main Feb 7, 2025
4 checks passed
@plexoos plexoos deleted the GMT branch February 7, 2025 15:04
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