Cytosim Scaling Bug #194
Cytosim Scaling Bug #194
Conversation
ascibisz
requested review from
rugeli and
toloudis
and removed request for
a team
toloudis
left a comment
toloudis
left a comment
There was a problem hiding this comment.
LGTM! Thank you for the quick fix. Too bad we don't have "simpler" test data so it's easier to see how the tests work.
| # value of automatically generated scale factor, so that position | ||
| # data fits within VIEWER_DIMENSION_RANGE | ||
| range = 0.400052 - -0.001 | ||
| range = 0.00808633 - -0.39575567 |
There was a problem hiding this comment.
curious why these changed? they used to be much cleaner looking numbers...is this a result of the bugfix?
There was a problem hiding this comment.
I'm not sure how I came up with the other numbers previously, but the new ones are the actual min / max numbers that define the range. I just noticed this as I was testing, since I was also in the weeds on testing the autoscaling
| used_unique_IDs: List[int], | ||
| overall_line: int, | ||
| total_lines: int, | ||
| scale_factor: float = None, |
There was a problem hiding this comment.
Just curious: what's deciding the scale_factor should be a float or not
There was a problem hiding this comment.
So scale_factor is an optional input parameter that the user may or may not provide when setting up their CytosimData object. Then, in TrajectoryConverter.scale_agent_data(), which I didn't change in this PR, if scale_factor is None, we generate a scale factor to get the data to look decent in the viewer (so like not be so tiny you can't see anything or so huge that it's out of frame).
Time estimate or Size
small
Problem
A user reported that scaling wasn't working as expected for the Cytosim converter.
Looking at the code, we were erroneously scaling all agent data that had been read thus far with each file read. So for example, for the user that reported the bug, there were 4 input files, so the first file that was read would get scaled by the scale factor 4 times, the second would get scaled 3 times, the third would get scaled twice, and the final file would get scaled the expected one time. This resulted in things looking very weird and unexpected!
Link to story or ticket
Solution
Let's just apply the scale factor once per conversion, after we have all the data read in. This is a very small change, just moving the scale factor part to be after the for loop where we're going through the input files.
Also let's update the test to make sure this doesn't happen again. Our original scale factor test only read in one file, so thus the problem with multiple files was missed.