Add ability to manually specify invalid detectors #64
Conversation
MialLewis
force-pushed
the
59_5_manually_enter_invalid_detectors
branch
from
1293de7
to
cc58cbb
Compare
| raise AttributeError("desired range invalid - must represent either front or back detectors.") | ||
|
|
||
| @staticmethod | ||
| def _preset_invalid_detectors(invalid_detector_list_full_range, desired_range): |
There was a problem hiding this comment.
It is common practice to reserve functions that start with an underscore for private members. Therefore, I recommend removing the underscore from any static members. It's your call, this is just a suggestion.
There was a problem hiding this comment.
I'm going to leave this - I added the underscore as I don't intend for this function to be called outside of the class despite it being static (maybe it shouldn't be, but it doesn't need to refer to any members etc.)
| raise AttributeError("Spec list invalid - must represent either front or back detectors.") | ||
|
|
||
| @staticmethod | ||
| def _print_invalid_detectors(invalid_detectors, detector_range): |
There was a problem hiding this comment.
This is also a personal preference. In my opinion, it would be more natural to have this function as a private class member. To achieve this, we could modify the invalid_detectors to a boolean parameter, where True indicates front and False indicates back. Personally, I prefer to avoid using static members in class definitions whenever possible.
There was a problem hiding this comment.
I've made this change - out of interest why do you prefer to avoid static members, I see conflicting advise regarding this?
There was a problem hiding this comment.
It is just a personal preference. The use cases of the static function are limited as it cannot access class members. The only common use case I think is factory functions. I think in most cases it is better to extract the function as an external function. In your scenario, since all arguments of the function are private variables and it is only used internally, it is better from an encapsulation perspective to hide the function. I'm also interested to hear your perspective.
There was a problem hiding this comment.
I don't have a strong opinion really. If I have a class member which does not access private member variables, if it is a function only likely to be used within that class I generally keep it within that class and mark it static. The static decorator just signals to the reader that it will not mutate any class members.
Description of work:
This PR introduces a method to manually specify invalid detectors.
add_invalid_detectorsto add additional detectors if neccessary.To test:
Check all unit and system tests pass.
Resolves #41