461 log normalization method#462
Conversation
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||
tE3m
left a comment
There was a problem hiding this comment.
One larger question regarding the execution of the correction, apart from that just some nitpicks that are up to you. Solid work overall, I would've forgotten about the runner
| # without adding the global median our data would be zero centered and therefore one half would be | ||
| # negative which can lead to problems later down the workflow | ||
| df_sample[f"Normalised {intensity_name}"] = ( | ||
| df_sample[intensity_name] - quantile + global_median |
There was a problem hiding this comment.
I'm assuming you did, but just to be certain: this is the behavior Chris wants for the normalization?
I tried changing the order (first doing the median normalization and then log-transformation) and the result:
is the same as if we didn't correct for the global median during the normalization (i.e. df_sample[intensity_name] - quantile instead of df_sample[intensity_name] - quantile + global_median):
Just for reference, this is the output with correction enabled:

So if this is the behavior Chris wants ("the users should know this"), I feel there should at least be a warning (or another checkbox). These steps being non-commutative would be surprising to me as a user
| intensity_name | ||
| ].div(quantile) | ||
| else: | ||
| df_sample[f"Normalised {intensity_name}"] = 0 |
There was a problem hiding this comment.
nitpick, code style: feels like the column name could be extracted to its own variable, the f-string definition is repeated for every of the 4 cases
| if self.form["log"].value: | ||
| self.form["visual_transformation"].value = ( | ||
| VisualTransformations.LINEAR.value | ||
| ) | ||
| elif not self.form["log"].value: | ||
| self.form["visual_transformation"].value = VisualTransformations.LOG10.value |
There was a problem hiding this comment.
nitpick, code style: two comments on this - I always feel that it's more legible to first define the form fields as variables in the modifiy_form methods. Secondly, since the enums are StrEnum instances, there's no need to access .value - VisualTransformations.LOG10 == VisualTransformations.LOG10.value is True.
Both non-critical and up to you, added a proposal for convenience
| if self.form["log"].value: | |
| self.form["visual_transformation"].value = ( | |
| VisualTransformations.LINEAR.value | |
| ) | |
| elif not self.form["log"].value: | |
| self.form["visual_transformation"].value = VisualTransformations.LOG10.value | |
| visual_transformation_field: DropdownField = self.form["visual_transformation"] | |
| log_field: CheckboxField = self.form["log"] | |
| if log_field.value: | |
| visual_transformation_field.value = ( | |
| VisualTransformations.LINEAR | |
| ) | |
| elif not log_field.value: | |
| visual_transformation_field.value = VisualTransformations.LOG10 |
Description
fixes #461
Adds another version of the normalization by median method.
Changes
Made changes to the method, added option to choose the method in forms and added and adapted tests.
Testing
Create a standard workflow, run it with checking the box. Switch the order and run it without checking the box.
PR checklist
Development
Mergeability
blackpnpm formatand checked withpnpm lintCode review