-
Notifications
You must be signed in to change notification settings - Fork 84
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
Stopmuon func in utils, add labels to i3extractor #148
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Leon! Nice Job!
Have added comments.
src/graphnet/data/utils.py
Outdated
#to do:remove hard-coded border coords and replace with GCD file contents using string no's | ||
border_coords = np.array([(-256.1400146484375, -521.0800170898438), (-132.8000030517578, -501.45001220703125), (-9.13000011444092, -481.739990234375), (114.38999938964844, -461.989990234375), (237.77999877929688, -442.4200134277344), (361.0, -422.8299865722656), (405.8299865722656, -306.3800048828125), (443.6000061035156, -194.16000366210938), (500.42999267578125, -58.45000076293945), (544.0700073242188, 55.88999938964844), (576.3699951171875, 170.9199981689453), (505.2699890136719, 257.8800048828125), (429.760009765625, 351.0199890136719), (338.44000244140625, 463.7200012207031), (224.5800018310547, 432.3500061035156), (101.04000091552734, 412.7900085449219), (22.11000061035156, 509.5), (-101.05999755859375, 490.2200012207031), (-224.08999633789062, 470.8599853515625), (-347.8800048828125, 451.5199890136719), (-392.3800048828125, 334.239990234375), (-437.0400085449219, 217.8000030517578), (-481.6000061035156, 101.38999938964844), (-526.6300048828125, -15.60000038146973), (-570.9000244140625, -125.13999938964844), (-492.42999267578125, -230.16000366210938), (-413.4599914550781, -327.2699890136719), (-334.79998779296875, -424.5)]) | ||
border_z = [-512.82,524.56] | ||
border = mpath.Path(border_coords) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we refactor this and perhaps change the variable naming?
It looks like border_coords
is indeed xy-coordinates, and if this is true, then lets please rename to border_xy
.
It would be nice if we could add a single input parameter borders
to the truth extractor that defaults to this choice.
We could save the border input variable as self._borders
and then
if self._borders == None:
border_xy = np.array([(-256.1400146484375, -521.0800170898438), (-132.8000030517578, -501.45001220703125), (-9.13000011444092, -481.739990234375), (114.38999938964844, -461.989990234375), (237.77999877929688, -442.4200134277344), (361.0, -422.8299865722656), (405.8299865722656, -306.3800048828125), (443.6000061035156, -194.16000366210938), (500.42999267578125, -58.45000076293945), (544.0700073242188, 55.88999938964844), (576.3699951171875, 170.9199981689453), (505.2699890136719, 257.8800048828125), (429.760009765625, 351.0199890136719), (338.44000244140625, 463.7200012207031), (224.5800018310547, 432.3500061035156), (101.04000091552734, 412.7900085449219), (22.11000061035156, 509.5), (-101.05999755859375, 490.2200012207031), (-224.08999633789062, 470.8599853515625), (-347.8800048828125, 451.5199890136719), (-392.3800048828125, 334.239990234375), (-437.0400085449219, 217.8000030517578), (-481.6000061035156, 101.38999938964844), (-526.6300048828125, -15.60000038146973), (-570.9000244140625, -125.13999938964844), (-492.42999267578125, -230.16000366210938), (-413.4599914550781, -327.2699890136719), (-334.79998779296875, -424.5)])
border_z = [-512.82,524.56]
border = mpath.Path(border_xy)
else:
border_xy = borders[0]
border_z = borders[1]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely agree, although I left the path.Path to the utils function so no need to import matplotlib in i3extractor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks @BozianuLeon
Hi @RasmusOrsoe and @BozianuLeon, I think this PR is a very nice contribution, and I am happy to see it merged! :) However, I would point out that several automated checks failed (see the "Checks" tab above), which means that following this PR, in principle, you cannot install and run this package out of the box on a clean system; something I figure we should otherwise strive for. (It also means that all subsequent PRs will report failing automated checks, potentially obscuring other errors, until the issue is resolved.) The problem is trivial — |
Hej Andreas
Jeg er helt enig (og lidt vild med) disse checks, idet vi virkelig gerne vil have, at man kan køre det “out of the box”.
Af samme grund, så synes jeg, at vi skal se på Mortens “case”, idet han fik en masse fejl. Det er nok ikke pga. pakken, men nok snarere fra hans/systemets side… men det er stadig god læring, for han er forhåbentlig ikke den sidste, som checker det ud, men desværre nok heller ikke den sidste, som løber ind i en eller anden form for problemer (han havde gcc probs).
BH. Troels
… On 6 Feb 2022, at 23.01, Andreas Søgaard ***@***.***> wrote:
Hi @RasmusOrsoe <https://github.com/RasmusOrsoe> and @BozianuLeon <https://github.com/BozianuLeon>,
I think this PR is a very nice contribution, and I am happy to see it merged! :)
However, I would point out that several automated checks failed (see the "Checks" tab above), which means that following this PR, in principle, you cannot install and run this package out of the box on a clean system; something I figure we should otherwise strive for. (It also means that all subsequent PRs will report failing automated checks, potentially obscuring other errors, until the issue is resolved.)
The problem is trivial — matplotlib is imported in the code, but not added to setup.py as a dependency — so I think we should just try to get a slim PR in to fix this issue as soon as possible. This is mainly just to point out the purpose of these pesky li'l checks. :)
—
Reply to this email directly, view it on GitHub <#148 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADLMFXLREEFWFHMYTOTHWH3UZ3VUHANCNFSM5M6XHTGQ>.
Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you are subscribed to this thread.
|
Yes, of course. Thanks for the heads up Andreas. If no one else has done this I will go ahead and add a matplotlib dependency in setup.py. (also good to know for future more complicated check failures) |
Finally adding functionality to calculate finishing point of muons, using starting position, track length, azimuth and zenith.
Would like to remove hard-coded border-coords using GCD file already opened(?). Will require recreating databases.