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

implement distance to surface and add the surface types to HPGe object #51

Merged
merged 31 commits into from
Nov 14, 2024

Conversation

tdixon97
Copy link
Contributor

@tdixon97 tdixon97 commented Nov 1, 2024

This implements a calculation of the distance between points and the surface.

  • methods for distance to surface for cylindrical detectors,
  • tests
  • add an attribute to HPGe class of the surface type,
  • adds method to compute surface area
  • distance to different surface (n+,p+ etc.)
  • check on if point is inside / outside (attach a direction to the distance),
    Also slightly refactors definitions of the surface for some detector types so that if there is no groove the p+ surface is still always separate from the others.

We should also add.

  • generalization to asymmetric detectors (including the surface types), add later. Only P00664A, V02160A

But these could be added later since the correct exceptions are raised in these cases currently.

Copy link

codecov bot commented Nov 1, 2024

Codecov Report

Attention: Patch coverage is 68.51852% with 85 lines in your changes missing coverage. Please review.

Project coverage is 77.86%. Comparing base (a7c9b2c) to head (ad7b135).
Report is 64 commits behind head on main.

Files with missing lines Patch % Lines
src/legendhpges/utils.py 53.22% 29 Missing ⚠️
src/legendhpges/draw.py 0.00% 22 Missing ⚠️
src/legendhpges/base.py 91.80% 5 Missing ⚠️
src/legendhpges/p00664b.py 54.54% 5 Missing ⚠️
src/legendhpges/ppc.py 54.54% 5 Missing ⚠️
src/legendhpges/invcoax.py 73.33% 4 Missing ⚠️
src/legendhpges/v02160a.py 73.33% 4 Missing ⚠️
src/legendhpges/v07646a.py 75.00% 4 Missing ⚠️
src/legendhpges/semicoax.py 72.72% 3 Missing ⚠️
src/legendhpges/bege.py 83.33% 2 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #51      +/-   ##
==========================================
- Coverage   79.74%   77.86%   -1.88%     
==========================================
  Files          14       16       +2     
  Lines         548      768     +220     
==========================================
+ Hits          437      598     +161     
- Misses        111      170      +59     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tdixon97 tdixon97 marked this pull request as draft November 1, 2024 17:40
@ManuelHu
Copy link
Collaborator

ManuelHu commented Nov 2, 2024

just an idea: maybe it would make sense to split utils into two parts; one that creates geometry (i.e. helpers for contacts, maybe spherical segments later on) and one for consuming geometry (i.e. calculating distances)?

@tdixon97
Copy link
Contributor Author

tdixon97 commented Nov 2, 2024

Yes probably you are right

@tdixon97 tdixon97 marked this pull request as ready for review November 2, 2024 22:33
@gipert
Copy link
Member

gipert commented Nov 4, 2024

Another thing to take care of: I think that because of finite numerical precision some hits could be found outside the detector by a tiny amount -> we should push them back in

Copy link
Collaborator

@ManuelHu ManuelHu left a comment

Choose a reason for hiding this comment

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

just some minor stuff I found while looking at the docs

src/legendhpges/base.py Outdated Show resolved Hide resolved
src/legendhpges/base.py Outdated Show resolved Hide resolved
src/legendhpges/utils.py Outdated Show resolved Hide resolved
@tdixon97
Copy link
Contributor Author

tdixon97 commented Nov 4, 2024

Another thing to take care of: I think that because of finite numerical precision some hits could be found outside the detector by a tiny amount -> we should push them back in

This is not necessarily so trivial, need to compute if a point is inside vs outside, i.e. attach a sign to the distance.

Copy link
Member

@gipert gipert left a comment

Choose a reason for hiding this comment

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

I left some comments.

All of this can be very easily sped up with Numba, let's do that.

src/legendhpges/base.py Outdated Show resolved Hide resolved
src/legendhpges/base.py Outdated Show resolved Hide resolved
src/legendhpges/base.py Outdated Show resolved Hide resolved
src/legendhpges/base.py Outdated Show resolved Hide resolved
src/legendhpges/utils.py Outdated Show resolved Hide resolved
src/legendhpges/utils.py Outdated Show resolved Hide resolved
src/legendhpges/utils.py Outdated Show resolved Hide resolved
src/legendhpges/invcoax.py Outdated Show resolved Hide resolved
src/legendhpges/p00664b.py Outdated Show resolved Hide resolved
src/legendhpges/utils.py Outdated Show resolved Hide resolved
src/legendhpges/base.py Outdated Show resolved Hide resolved
src/legendhpges/base.py Outdated Show resolved Hide resolved
@tdixon97
Copy link
Contributor Author

tdixon97 commented Nov 5, 2024

I left some comments.

All of this can be very easily sped up with Numba, let's do that.

Now done.

@tdixon97
Copy link
Contributor Author

Any more issues? Can we merge this ?

@ManuelHu
Copy link
Collaborator

for me it looks fine, we can always iterate later if we need to.

@gipert gipert self-requested a review November 12, 2024 15:38
@tdixon97
Copy link
Contributor Author

Should have now fixed your last comments @gipert

@gipert
Copy link
Member

gipert commented Nov 14, 2024

I have removed the pre-commit commits and pushed some cosmetic stuff, getting ready to merge

pre-commit-ci bot and others added 3 commits November 14, 2024 15:38
updates:
- [github.com/adamchainz/blacken-docs: 1.18.0 → 1.19.1](adamchainz/blacken-docs@1.18.0...1.19.1)
- [github.com/astral-sh/ruff-pre-commit: v0.6.9 → v0.7.2](astral-sh/ruff-pre-commit@v0.6.9...v0.7.2)
- [github.com/pre-commit/mirrors-mypy: v1.11.2 → v1.13.0](pre-commit/mirrors-mypy@v1.11.2...v1.13.0)
- [github.com/abravalheri/validate-pyproject: v0.20.2 → v0.22](abravalheri/validate-pyproject@v0.20.2...v0.22)
- [github.com/python-jsonschema/check-jsonschema: 0.29.3 → 0.29.4](python-jsonschema/check-jsonschema@0.29.3...0.29.4)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@gipert gipert merged commit 4c81bb7 into legend-exp:main Nov 14, 2024
15 checks passed
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