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

Taking into account Geiger hits with NaN radii #4

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

matteoceschia
Copy link

Hi Yorck,

this is a very first attempt to take into account Geiger hits with NaN radii. I went for the most conservative approach, i.e. simply discarding them. I ran and tested it, and it seems to work fine.
However, I can think of a couple of issues that might arise from discarding hits with NaN radii:

  • the "number of gg hits" is equal to gg_hits_col.size() and memory is reserved to the vector "rings" accordingly (rings.reserve(gg_hits_col.size()). Could discarding hits raise memory issues in C++? (I guess it shouldn't, as we're reserving a bit more memory than needed in such cases, but C++ always amazes me. Also it might be a bit inefficient in principle, but it seems NaN radii are not that common)

  • in my test sample, there was a cluster of 2 hits, but one had NaN radius so it was discarded. This raised the following errors, as one might expect:

Error in <TDecompChol::Decompose()>: matrix not positive definite
Error in <TDecompChol::Solve()>: Decomposition failed
Error in <TLinearFitter::Eval>: Matrix inversion failed

But then the code proceeded to compute the fits with no apparent issue.

Would this fix be ok, or would it be safer to create a &gg_hits_col object discarding NaN radii from the very beginning?

Cheers,
Matteo

@yramachers
Copy link
Collaborator

The PR looks fine, no problem with the oversized vector even though that could be changed. Don't remember why I chose to 'emplace' instead of 'push_back'. There is probably a reason for that since I normally wouldn't use that construction.

Anyway, the fitting of a single ring is not right. Annoyingly, I used to be more careful of such anomalies and catch them right at the start of functions but indeed I didn't, just checked. I reckon I prevented single ring clusters in the clustering and didn't consider catching that mistake in the fitter.

Yes, the fit will fail and be caught and show no adverse effect but it is still a mistake and a waste of time and should not happen. Could you add a catch clause at the start of the fitline(), ..helix(), ..brokenline() functions, please and return an empty vector if the data member 'rings' is too small (i.e. 1 for lines, 2 for a helix)? That would be an if statement in SNFitter.cpp at line 62, 263 and 441.

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.

2 participants