Feature/auto handle scale factor #148
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #148 +/- ##
==========================================
+ Coverage 92.56% 92.74% +0.18%
==========================================
Files 101 101
Lines 4546 4673 +127
==========================================
+ Hits 4208 4334 +126
- Misses 338 339 +1
☔ View full report in Codecov by Sentry. |
ascibisz
requested review from
blairlyons,
frasercl,
interim17 and
toloudis
and removed request for
a team
simulariumio/trajectory_converter.py
Outdated
| self.progress_callback(percent_complete) | ||
| self.last_report_time = current_time | ||
|
|
||
| def _filter_agent_data(data: np.array, n_agents: np.array): |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| else: | ||
| scale_factor = input_data.meta_data.scale_factor | ||
| for index in range(dimensions.total_steps): | ||
| # do not scale radii for subcells |
There was a problem hiding this comment.
why don't we want to scale subcell radii?
There was a problem hiding this comment.
oh goodness I don't remember, I think this came out of when I manually checked that all of the scaled results looked like what I would expect them to be. I'll double check, and if there is a good reason I'll make the comment more descriptive
- figure out why "do not scale radii for subcells"
There was a problem hiding this comment.
ok so I lied, I came across this because this matches the previous functionality so if i just did radii *= scale_factor for all radii, a previous test would fail. That's because "subcells" are represented as sphere groups, and so previously if someone had provided a scale factor, we wouldn't apply that to the agent of the sphere group's radii. I imagine it doesn't actually matter which way we do it, because I don't think the sphere group's agent radii matters? since the radii for each of the spheres of the sphere group is in the subpoints?
| assert box_size == expected_box_size | ||
|
|
||
|
|
||
| auto_scale_factor = VIEWER_DIMENSION_RANGE.MIN / (0.400052 + 0.001) |
There was a problem hiding this comment.
they're pulled from the test data, that represents the max range of the positional data, which determines the scale factor. I can pull that out and call it the range, so it's a little more clear?
- make range variable
… nth timestep together
ascibisz
force-pushed
the
feature/auto-handle-scale-factor
branch
from
6b3e497 to
f2cc98a
Compare
Problem
It's confusing for users to have to come up with an appropriate scale factor that'll look good in the simularium viewer. Instead, simulariumio should be able to choose an appropriate scale factor based on the spatial positions.
Link to ticket
Solution
Based on manually trying on different scale factors and seeing what positional ranges looked bad, I decided the viewer's ideal range is 5-70, so I made a new constant for that. In each of the converters, I kept track of the ranges of the XYZ coordinates when spatial data is being processed, and then come up with a scale factor accordingly. I then scale radii, positions, subpoints, and box size accordingly. If a scale factor was provided, that will still be used instead of the calculated one, to maintain previous functionality.
Type of change