RecommendedCutoffMixin: set only one stringency with set_cutoffs
#72
Conversation
Currently the behaviour of the `set_cutoffs` and `get_cutoffs` methods of the `RecommendedCutoffMixin` class is different. The first sets the cutoffs for all stringencies along with a default stringency, the second only retrieves the cutoffs for a specified stringency. This can be somewhat confusing for the user, and means that the user cannot easily set the cutoffs for a single stringency. Here we change the `set_cutoffs` method so it only sets the recommended cutoffs for a single stringency, which has to be specified as an input argument. The stringency is automatically set to be the default in case it is the first stringency specified, and a `set_default_stringency` method is added to the user can still change the default stringency at any time after setting multiple stringencies. We also allow the user to specify units for each stringency, extending the value corresponding to the `_key_cutoffs_unit` in the extras to a dictionary that specifies the units for each stringency, similar to the cutoffs.
|
@sphuber test should be passing, but could still do with a good scrubbin'. Will do so once we are agreed on the design! |
There was a problem hiding this comment.
Thanks @mbercx some minor adjustments but I think the design of the interface is good like this. Maybe just add some tests surrounding the fact that we now have multiple units and add a test on what should happen if you call it and no default stringency is set.
Co-authored-by: Sebastiaan Huber <[email protected]>
| except KeyError as exception: | ||
| raise ValueError(f'stringency `{stringency}` is not defined for this family.') from exception | ||
|
|
||
| def delete_cutoffs(self, stringency) -> None: |
There was a problem hiding this comment.
And maybe we do the same as in set_cutoffs, if after the operation is completed there is only one stringency left, that becomes the new default. If you agree, would be good to have a test for this and add in the docstring like for set_cutoffs
There was a problem hiding this comment.
I agree it makes sense, and don't immediately see any issue with this. I wonder if we should print a message when the user removes the default_stringency with the remove_cutoffs method. Something along the lines of:
- Only one stringency remaining:
The default stringency has been set to: '<LEFTOVER STRINGENCY>' - Multiple stringencies remaining:
Default stringency removed but multiple options left. Please set the new default stringency with the ``set_default_stringency()`` method.
And I will make sure that Oliver's hunger for tests is amply satisfied. 😁
There was a problem hiding this comment.
Would be fine with me, but what are you suggesting to use for printing the message? I think the only options we have are logging or a warnings.warn and probably the latter since we haven't really used or setup logging yet. Printing directly I don't like in normal API code tbh.
|
That should turn Oliver's frown upside down! I also implemented the logic discussed and added some warnings in case the default stringency is removed, or a new default stringency needs to still be set. |
Fixes #68
Currently the behaviour of the
set_cutoffsandget_cutoffsmethods of theRecommendedCutoffMixinclass is different. The first sets the cutoffs for allstringencies along with a default stringency, the second only retrieves the
cutoffs for a specified stringency. This can be somewhat confusing for the
user, and means that the user cannot easily set the cutoffs for a single
stringency.
Here we change the
set_cutoffsmethod so it only sets the recommendedcutoffs for a single stringency, which has to be specified as an input
argument. The stringency is automatically set to be the default in case
it is the only stringency specified, and a
set_default_stringencymethod is added to the user can still change the default stringency at
any time after setting multiple stringencies.
We also allow the user to specify units for each stringency, extending
the value corresponding to the
_key_cutoffs_unitin the extras to adictionary that specifies the units for each stringency, similar to the
cutoffs.