-
Notifications
You must be signed in to change notification settings - Fork 3
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
some new functions to help playing with bias- and trigger-patches #21
Conversation
This breaks python 3.3 compatibility (because pandas 0.18 require 3.4). I think we should drop the 3.3 test from travis. |
trigger_patch_currents = t_c # unshorten the name | ||
return trigger_patch_currents | ||
|
||
def take_apart_trigger_values_for_bias_patches(trigger_rates): |
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.
trigger_rates? This valid for every array of length 160, right? Isn't this simply converting trigger_patch_id
-> bias_patch_id
?
Would jsut two conversion methods trigger_patch_to_bias_patch
and bias_patch_to_trigger_patch
be sufficient and more general?
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.
I think, There is no general way to combine or take apart these values ... combining could be:
- sum them,
- sum them with weights 4/9 and 5/9
- take their max
- ...
taking apart could be:
- simply duplicate them, as I did there
- divide by 2
- divide into 4-ninth and 5-ninth
- ...
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.
I think before making stuff general ... let's just implement features we need .. then add more features ... and once we find some features have overlapping code, we refactor in order to increase code reuse ... let's not search for generality ... at least that's how I understood uncle bob.
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.
But you implemented a function that does the general id conversion AND something else. That's not uncle-bobish
return pm | ||
|
||
@lru_cache(maxsize=1) | ||
def patch_indices(): |
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.
I think functions should be verbs. get_patch_indices
or read_patch_indices
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.
I think this should not even be a function, but a module global variable like, pixel_mapping
. What do you think?
but not having it is much worse than than not having it machine readable.
this has to be in pyfact
I think so too |
GEOM_2_SOFTID = { | ||
(i, j): soft for i, j, soft in zip( | ||
pixel_mapping['geom_i'], pixel_mapping['geom_j'], pixel_mapping['softID'] | ||
)} | ||
|
||
|
||
@lru_cache(maxsize=1) | ||
def pixel_dataframe(): |
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.
This should either be a member or have another name (verb)
Now the tests are failing, I think you forgot to rename the functions in the tests |
Looks good to me! |
We have twice as many bias-patches as trigger-patches in FACT, but their mapping in non trivial.
In addition when trying to combine the two "bias_patch_currents" in the one "trigger_patch_current" one needs even to know which of the two patches had 4 pixels and which had 5...
This all repeatedly kicked me in the balls so I thought it should go in here.
I also added some tests, but went crazy while writing them ... so this is not yet done, but I need it on newdaq, so I pushed it ...
I guess the functions should not go into
fact.pixels
but somewhere else ... however implemplementing it there felt so natural, I couldn't resist.Looking forward to your feedback.