cleanup position unit #733
Conversation
rpauszek
requested review from
tommasogritti,
tobiasjj and
JoepVanlier
and removed request for
a team
| unit=f"{kymotracks._calibration.unit}^2 / s", | ||
| _unit_label=f"{kymotracks._calibration.unit.label}²/s", |
There was a problem hiding this comment.
Forgot to replace it with as_ function?
lumicks/pylake/kymo.py
Outdated
| def as_diffusion(self): | ||
| return { | ||
| "unit": f"{self}^2 / s", | ||
| "_unit_label": f"{self.label}²/s", | ||
| } | ||
|
|
||
| def as_squared(self): | ||
| return { | ||
| "unit": f"{self}^2", | ||
| "_unit_label": f"{self.label}²", | ||
| } |
There was a problem hiding this comment.
I feel a bit conflicted about these helper functions. I do like that it is in one place now. It does feel a little odd to put this in the PositionUnit itself. For now this is probably OK and an improvement from what we had before, but if we get more of these in the future, we might want to consider making a unit formatter or something like that, to separate the logic and not pollute the unit class too much with implementations for specific situations. In this case, the output feels very specifically tailored to those dataclasses we have in the diffusion module. It's an improvement from what we had before though.
A second thing is the naming. The as_ would imply to me that I'd be getting a new representation of this unit, rather than labels for a different unit alltogether. I'd sooner call it get_*_labels or something along those lines to make that a little more clear.
I think it would also make sense to add a type annotation for the return type in this case (as it is not immediately clear to me from the name).
Sorry for all the nitpicking. 😅
There was a problem hiding this comment.
I do wonder whether it isn't time to rope in an actual unit library to track units for us in these cases so that we also get composite units. Considering that _unit_labelis private in all the dataclass input args, one could consider changing these. You could have them take a UnitInfo | String directly for the unit argument and unpack to add the unit as string and the _unit_label in __post_init__. If we ensure that it can take both strings and units that shouldn't be breaking. That said, that would make more sense if we actually had composite units, because as it stands right now, you'd still need getters of the type you implemented, so this is probably not something to do already now.
There was a problem hiding this comment.
Yeah I agree with pretty much all of this and I had tried going down that route initially but it got pretty complicated pretty fast. I don't particularly like these small super-specific methods, but as far as I can see we only need something like this for the diffusion stuff at the moment so figured this way was the biggest payoff for the least amount of effort. I had thought about changing up the DiffusionEstimate dataclass too but you can't process constructor arguments with __post_init__ for frozen dataclasses so that's a complication. I'm sure there's a way to get all of this working, but again for just this one instance so far it seemed wise to go with the path of least resistance.
I do really like the name suggestion though, so I did change that 👍
There was a problem hiding this comment.
Makes sense yeah. We can always introduce a different abstraction later once we know a few more places where we'd need similar functionality.
rpauszek
force-pushed
the
ray/calibrate-tether-upside-down
branch
from
5385da7 to
73effc8
Compare
rpauszek
force-pushed
the
ray/cleanup-position-unit
branch
2 times, most recently
from
1e82d2d to
d9ddbff
Compare
JoepVanlier
force-pushed
the
ray/cleanup-position-unit
branch
from
d9ddbff to
9ff03fa
Compare
Why this PR?
Now with
PositionUnitI thought it would be nice to cleanup and centralize everything having to do with position units for kymos, tracking and msd estimation. I think especially useful is the string formatting for diffusion constants handled as a method on the enum so we can guarantee consistency.