Various improvements to PHCASeeding #3434
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Functionally fixes the `PHCASeeding->SetSplitSeeds(false);` to work
as expected.
- Removes the separate map<TrkrDefs::cluskey,vector3> to just making
a single vector list of the {cluskey,vector} data with pointers
in the links. This is for efficiency, and works nicely. This
also saves recalculating phi many times.
- When adding clusters, don't continually recalculate phi, dphi,
R, dR, dzdR, d2phidr2, but only as needed. Again this is
for efficiency.
Still needed:
- The cluster still makes all combinations of all triplets
to start tracks. This should be either ignored (start further
down the chain) or squashed into average clusters.
- Have the seeds start in the middle, grow inward, then outward
Build & test reportReport for commit a6cabab5cfe6df22bf44f5ae32e64e972a6ce51c: Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
Author
|
Hi @pinkenburg / @osbornjd -- |
Contributor
|
Looks like the newer version of g++ picks up a possible error I would login to an alma9 node, try to compile there, and debug and push a commit |
Now compiles and runs. Also commentet out optional compile flag for debugging TNtuple output.
Build & test reportReport for commit a1cd61cc8792e3ba10f3dcd1cc882496bc9432b9:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
|
This improves the efficiency in the simulation pretty substantially, do you have some QA checks on the data too? |
Contributor
Author
Hey @osbornjd, I looked at the Jupyter QA plots earlier, and I do not understand why the efficiency is so much better. I was hoping that it was because of other changes in the process/distortion corrections that are happening concurrently. I had better check in data and see what happens, just to make sure it's not something that would blow up the combinatorics dramatically. |
Build & test reportReport for commit 4691a1240e52077096cfd6f8328cc3dccebc7036:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
Author
|
OK, the results on spot checking with actual data are identical. There is a complaint from Valgrind, but there isn't any dynamically allocated memory -- they are all allocated into std::vector. What is manual are pointers to that vector, but the memory can't actually be leaked, as both the vector of data, and the vector of pointers, are managed automatically. |
Contributor
Author
Contributor
|
How does this mesh with Michael's PR? Maybe we should come up with a plan for how to merge both and/or understand how they help/hurt each other |
Build & test reportReport for commit e5264807f5b73244ecc6495f0d9562db5b12fabd:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
Author
|
OK @petermic -- there are still some algorithmic choices to made regarding removing duplicate seeds, but I think that this successfully merges your PR code. So, if all is good, I think it is all ready to merge. |
Contributor
Author
Contributor
|
I may have missed something, but I thought @petermic still wanted to do some development on his PR? Because it halved the efficiency in AuAu data |
Contributor
Author
|
ah, ok, that may be true. I haven't tested with the data outside of Jenkins. I'll talk with @petermic and maybe just merge this one with his and then the single PR can go in eventually. |
Contributor
|
Have you also run tests with this one on some data too? |
Contributor
Author
|
At least not after I merge in Michael's code with it |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PHCASeeding->SetSplitSeeds(false);to work as expected.Still needed:
down the chain) or squashed into average clusters.
Given that the default Fun4All macro splits the tracks, this shouldn't change the QA. I'll wait to see them before asking for the PR merge.
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)