Refactor readers iii #242
Refactor readers iii #242
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
| If str: the parameter will be interpreted as a reader type, and the modification_mapping is read from the | ||
| "modification_mapping" section of the psm_reader_yaml | ||
|
|
||
| """ | ||
| if modification_mapping is None: | ||
| self._init_modification_mapping() | ||
| elif isinstance(modification_mapping, str): |
There was a problem hiding this comment.
@jalew188 here, a lot of hidden complexity arises (also connected to _init_modification_mapping) as the "column_mapping" parameter in the yaml is overloaded (could either be str or dict).
Could we introduce a (mutually exclusive ) "column_mapping_type` in the yaml, that served to reference another reader?
There was a problem hiding this comment.
Yes, we shall then add a new method: add_new_column_mapping_item to handle user-defined columns
There was a problem hiding this comment.
Both modification_mapping and column_mapping are not well designed in alphabase readers. We need a better way.
| @@ -257,33 +265,39 @@ def set_modification_mapping( | |||
| else: | |||
| self.modification_mapping = copy.deepcopy(modification_mapping) | |||
|
|
|||
| self._mods_as_lists() | |||
| self._reverse_mod_mapping() | |||
| self._str_mods_to_lists() | |||
There was a problem hiding this comment.
Is there a reason why this attribute is mutated in place, while the others are reassigned?
(see e.g. line 269)
There was a problem hiding this comment.
I guess for readability? Complex Inplace operations are hard to read and track
There was a problem hiding this comment.
good catch @lucas-diedrich , I was wondering too .. it's as @jalew188 suspected indeed the limited complexity that made me keep str_mods_to_lists as in-place .. but more based on a gut feeling rather than on a strict reasoning .. I could change it for consistency if you like?
There was a problem hiding this comment.
I see, thank you for the explanation 👍 So I guess the alternative would then be something like
self.modification_mapping = _str_mods_to_lists(self.modification_mapping)With
def _str_mods_to_lists(self, modification_mapping: dict) -> dict[str, list[str]]:
"""Convert all single strings to lists containing one item in self.modification_mapping."""
for mod, val in list(modification_mapping.items()):
if isinstance(val, str):
modification_mapping[mod] = [val]
return modification_mappingI don't have a strong opinion on this, and the current way looks great, so no need to change it!
simplify the modification mapping handling
(see individual commits for details)