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

feat: Added a function that draws the detector volume with inner surfaces #1946

Merged
merged 5 commits into from
Mar 16, 2023

Conversation

dimitra97
Copy link
Contributor

Added the drawDetectorVolumeWithSurfaces function that draws the detector volume with the sub-volumes and their surfaces . Any comments and recommendations are more than welcome.
@andiwand @Matthewharri @asalzburger

@dimitra97 dimitra97 changed the title Added a function that draws the detector volume with inner surfaces feat: Added a function that draws the detector volume with inner surfaces Mar 15, 2023
@codecov
Copy link

codecov bot commented Mar 15, 2023

Codecov Report

Merging #1946 (f67221b) into main (95e3d9a) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1946      +/-   ##
==========================================
- Coverage   49.59%   49.57%   -0.02%     
==========================================
  Files         410      410              
  Lines       22697    22705       +8     
  Branches    10358    10366       +8     
==========================================
  Hits        11257    11257              
- Misses       4228     4236       +8     
  Partials     7212     7212              
Impacted Files Coverage Δ
Core/src/Visualization/GeometryView3D.cpp 32.11% <0.00%> (-1.08%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions
Copy link

github-actions bot commented Mar 15, 2023

📊 Physics performance monitoring for f67221b

Full report
Seeding: seeded, truth estimated, orthogonal
CKF: seeded, truth smeared, truth estimated, orthogonal
IVF: seeded, truth smeared, truth estimated, orthogonal
Ambiguity resolution: seeded, orthogonal
Truth tracking
Truth tracking (GSF)

Vertexing

Vertexing vs. mu
IVF seeded

IVF truth_smeared

IVF truth_estimated

IVF orthogonal

Seeding

Seeding seeded

Seeding truth_estimated

Seeding orthogonal

CKF

CKF seeded

CKF truth_smeared

CKF truth_estimated

CKF orthogonal

Ambiguity resolution

seeded

Truth tracking (Kalman Filter)

Truth tracking

Truth tracking (GSF)

Truth tracking

Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

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

I wonder if we should combine this with drawDetectorVolume somehow or just create a new function as you did. maybe @Matthewharri has some input for us

other than that you should reformat the code using clang-format so it aligns with the rest of the code and the CI will stop complaining

@paulgessinger paulgessinger added this to the next milestone Mar 15, 2023
@Matthewharri
Copy link
Contributor

I think it might be best to have it combined with drawDetectorVolume somehow like @andiwand mentioned. But I do have a question, if you're drawing the portals, shouldn't that be all that is needed to draw the DetectorVolume? Or am I missing something obvious?

@dimitra97
Copy link
Contributor Author

I think it might be best to have it combined with drawDetectorVolume somehow like @andiwand mentioned. But I do have a question, if you're drawing the portals, shouldn't that be all that is needed to draw the DetectorVolume? Or am I missing something obvious?

@dimitra97 dimitra97 closed this Mar 16, 2023
@dimitra97 dimitra97 reopened this Mar 16, 2023
@andiwand
Copy link
Contributor

I think it might be best to have it combined with drawDetectorVolume somehow like @andiwand mentioned. But I do have a question, if you're drawing the portals, shouldn't that be all that is needed to draw the DetectorVolume? Or am I missing something obvious?

thanks for the feedback @Matthewharri ! each detector volume has a list of portals and surfaces where the surfaces represent material or sensitive elements of the detector. is it okay for you if we plot all the surfaces all the time or should we flag it so you can also just plot the portals as it used to be

@Matthewharri
Copy link
Contributor

@andiwand Thanks for the explanation, that makes sense. I think it should be fine either way, so really whatever you prefer

@dimitra97 dimitra97 force-pushed the feat-volume-surf-vis branch from 27c9275 to e8325fe Compare March 16, 2023 17:19
@andiwand
Copy link
Contributor

@dimitra97 if you apply my suggested change we can merge this in

@dimitra97
Copy link
Contributor Author

dimitra97 commented Mar 16, 2023

@dimitra97 if you apply my suggested change we can merge this in

sure! i applied

Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

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

👍

@andiwand andiwand added automerge Improvement Changes to an existing feature Impact - Minor Nuissance bug and/or affects only a single module labels Mar 16, 2023
@kodiakhq kodiakhq bot merged commit 6b86cef into acts-project:main Mar 16, 2023
paulgessinger pushed a commit to paulgessinger/acts that referenced this pull request Mar 23, 2023
…aces (acts-project#1946)

Added the `drawDetectorVolumeWithSurfaces` function that draws the detector volume with the sub-volumes and their surfaces . Any comments and recommendations are more than welcome. 
@andiwand @Matthewharri @asalzburger
@paulgessinger paulgessinger removed this from the next milestone Mar 31, 2023
@paulgessinger paulgessinger added this to the v24.0.0 milestone Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Impact - Minor Nuissance bug and/or affects only a single module Improvement Changes to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants