-
Notifications
You must be signed in to change notification settings - Fork 26
Update repo to work on NEXT-100 #54
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
Conversation
ee05245 to
cdc3588
Compare
a66b78e to
cdbc060
Compare
bpalmeiro
left a comment
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.
First round of comments. Good job so far!
b71295e to
5a70212
Compare
bpalmeiro
left a comment
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.
Long-needed update for the code to run detector independently. Changes are profound and sensible, improving also flaky loose ends pending from before. Good job.
For some reason time evolution was tested separately
02f8a07 to
6837ff4
Compare
|
rebased. I'm trying to fix the issue with GHA, but this is ready to be merged. |
|
Ignore GHA failure, please merge. |
6837ff4 to
0186efc
Compare
#55 [author: gonzaponte] We pushed through #54 to get everything asap. Now we fix the tests. The results seem to depend on the machine somehow, so for now this is ignored. [reviewer: carhc] Nice job! I have one small issue, thought. I suggest removing the x_range, y_range in the config files since they no longer play any role on the evolution computation, but maybe this is not the most suitable PR for that since that does not affect the tests. Other than that, I think it's just the commented assert on test_scrip_runs_and_produces_correct_outputs, apparently machine-dependent. How should we proceed now?
Includes a hack for NEXT-100, since the table DETECTOR_GEO is currently empty on the database.