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

Deprecate ECal cellxy.txt and use EcalGeometry instead #1425

Merged
merged 5 commits into from
Sep 4, 2024

Conversation

tvami
Copy link
Member

@tvami tvami commented Aug 31, 2024

I am updating ldmx-sw, here are the details.

What are the issues that this addresses?

Resolves #1413

The code certainly got much simpler like this, no weird stepping in the mapsx/mapsy anymore.

Check List

  • I successfully compiled ldmx-sw with my developments
  • I ran my developments and the following shows that they are successful.

As a test I fixed the X location to -9999, or to -270 --> indeed it shows that it's not fiducial.
Otherwise ran the new kaon config, the fiducial case looked ok too.
I expect the bigger effects to show up in signal, especially with the 1 GeV case.

@tvami tvami force-pushed the iss1413-deprecate-cellxy branch from 111923a to 535af36 Compare August 31, 2024 20:23
@tvami
Copy link
Member Author

tvami commented Aug 31, 2024

@tomeichlersmith I think we should cut gold and then have this PR in, like that it nicely separates from other developments (like the lin-reg changes)

Copy link
Member

@tomeichlersmith tomeichlersmith left a comment

Choose a reason for hiding this comment

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

This is an excellent update and one that a student I'm working with also has a use for!

My only comment is about reducing duplication.

DetDescr/src/DetDescr/EcalGeometry.cxx Outdated Show resolved Hide resolved
@tvami tvami force-pushed the iss1413-deprecate-cellxy branch from 175575e to 542e85a Compare September 3, 2024 20:03
Copy link
Member

@tomeichlersmith tomeichlersmith left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

The last thing is just to update the documentation of getID to include the fallible parameter. And maybe an example about how to use it, e.g.

(doxycomments can contain codeblocks just like markdown).

auto id = geometry.getID(x, y, ilayer, true);
if (id.null()) {
  // position (x,y) is not contained within a cell in layer ilayer
}

@tvami tvami force-pushed the iss1413-deprecate-cellxy branch from 542e85a to 2645419 Compare September 4, 2024 00:37
@tvami tvami merged commit 18db944 into trunk Sep 4, 2024
4 of 10 checks passed
@tvami tvami deleted the iss1413-deprecate-cellxy branch September 4, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate ECal cellxy.txt and use EcalGeometry instead
3 participants