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

bring measurements back to trajectory #60

Merged
merged 4 commits into from
Jan 26, 2024
Merged

bring measurements back to trajectory #60

merged 4 commits into from
Jan 26, 2024

Conversation

ShujieL
Copy link
Contributor

@ShujieL ShujieL commented Nov 9, 2023

Briefly, what does this PR introduce?

Move measurements from Track (which is introduced in #58) back to Trajectory, and add back outliers.

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • New feature (issue #__)
  • Documentation update
  • [x ] Other: partially revert a change

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • [x ] Changes have been communicated to collaborators. -- This change is communicated to Joe Osborn.

Does this PR introduce breaking changes? What changes might users need to make to their code?

CKFtracking.cc needs to be updated to take the new data structure.

Does this PR change default behavior?

move measurements from Track back to Trajectory, and add back outliers.
@ShujieL ShujieL requested a review from a team as a code owner November 9, 2023 00:30
@ShujieL ShujieL requested a review from osbornjd November 9, 2023 00:30
@wdconinc
Copy link
Contributor

wdconinc commented Nov 9, 2023

We discussed this in Joe's talk. We don't want these hits in trajectory.

@osbornjd
Copy link
Contributor

osbornjd commented Nov 9, 2023

Shujie discussed with me offline and wanted this because (correct me if I'm wrong)

  1. It may be confusing to users who use this
  2. The differentiation between outlier measurements and track measurements is needed, even though outliers are a subset of the total measurements that belong to a track. Acts determines what a track state looks like with a flag (basically an enum) that labels it as measurement, outlier, hole, whatever. In principle doing something like this would consolidate the information.

I still personally think we should have the measurements in the Track object because analyzers may want to know how many measurements are on a track for quality cuts (e.g. if an analysis requires the best momentum resolution possible, require more measurements on the track). The point of differentiating between Track and Trajectory was to separate expert from user information, so that is why I put the measurement list in the Track object.

@wdconinc
Copy link
Contributor

wdconinc commented Nov 9, 2023

Shujie discussed with me offline and wanted this because

Whatever it is, let's try to get this right in fewer steps, ok? I.e. this better be the final one.

osbornjd
osbornjd previously approved these changes Nov 9, 2023
Copy link
Contributor

@osbornjd osbornjd left a comment

Choose a reason for hiding this comment

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

I don't have an issue with this, my opinion is that the measurements belong in the Track object as analyzers may want to know how many exist on a track (and how many from various subsystems exist), and if we are trying to keep analyzer info from expert info separate then this should be moved. But I'm not going to argue for it if others think it is better in this proposed way

@ShujieL
Copy link
Contributor Author

ShujieL commented Jan 23, 2024

I noticed this PR is still open.
The "Track" data structure is not used anywhere in EICrecon. All ongoing analysis access measurements etc from "Trajectory" with the previous tagged version. To be consistent, can we keep this trajectory structure unchanged (i.e. bring the references to hits back as I suggested in this PR). We can always change this when future algorithm development with "Track" is ready.
@wdconinc @osbornjd

@ShujieL ShujieL requested a review from osbornjd January 23, 2024 18:54
@wdconinc
Copy link
Contributor

The "Track" data structure is not used anywhere in EICrecon.

I think we(you) should address that issue instead of reverting data model changes.

@osbornjd
Copy link
Contributor

It's fine to merge this to maintain current functionality IMO but it should not be used as an excuse to punt the switch to Track further down the road. The longer it takes the more entrenched analysis will be in the current scheme which is not maintainable long term...

edm4eic.yaml Outdated Show resolved Hide resolved
edm4eic.yaml Outdated Show resolved Hide resolved
@ShujieL ShujieL merged commit c729b43 into main Jan 26, 2024
3 checks passed
@ShujieL ShujieL deleted the trajectory branch January 26, 2024 02: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.

3 participants