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

Iss883 #888

Merged
merged 5 commits into from
Sep 10, 2021
Merged

Iss883 #888

merged 5 commits into from
Sep 10, 2021

Conversation

pbutti
Copy link
Contributor

@pbutti pbutti commented Sep 8, 2021

  • Added front-back fitting of tracks with KF
  • Fixes to KF pattern reco

…cut; fixed error causing same track candidate to be found several times in some cases; fixed some debug format errors; added lots of debugging code for studying this issue
@JeremyMcCormick
Copy link
Member

JeremyMcCormick commented Sep 9, 2021

Just a quick tip based on scanning your code:

If you want your comments to show up as documentation in the javadoc attached to classes and methods then enclose them like this:

/**
  * This will show up in the javadoc!
  */
class MyClass {
    /**
      * So will this!
      */
      void something() {}
}

Double-slash comments will not:

// This will not show up in the javadoc. :(
void something() {}

Non-javadoc style is more appropriate for internal comments within methods.

Just mentioning this because you have some documentation-style comments on classes and methods but they will be stripped out if someone is looking at the online javadoc and won't be visible at all there.

@JeremyMcCormick
Copy link
Member

JeremyMcCormick commented Sep 9, 2021

Also, a few more stylistic nitpicks:

We don't typically prepend underscores in Java code like this within this codebase:

_materialManager

Instead to clearly denote a class variable you can just use this.materialManager within the code. This will use the class variable even if materialManager was another variable declared in the method scope.

And here you may want to extract some of these hard-coded numbers out into constants:

Double chi1 = new Double(p1*t1.chi2s / t1.hits.size() + 300.*(1.0 - (double)t1.hits.size()/14.));

You can then change them easily in one spot and their meaning becomes much clearer to anyone reading the code.

Copy link
Contributor

@normangraf normangraf left a comment

Choose a reason for hiding this comment

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

I've not done an in-depth review, but all tests pass.
Please proceed.

@pbutti pbutti merged commit 4771287 into master Sep 10, 2021
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