search

simularium / simulariumio

Python package that converts simulation outputs to the format consumed by the Simularium viewer website
Apache License 2.0
5 stars 3 forks source link

Append multiple agents to TrajectoryData objects #155

Closed mogres closed 1 year ago

mogres commented 1 year ago

Problem

Refer to #153

Solution

Type of change

Steps to Verify:

Refer to this notebook which creates a single simularium file from multiple cytosim simulations

Screenshots:

Simularium trajectory with multiple actin filaments visualized simultaneously image

Keyfiles:

simulariumio/data_objects/trajectory_data.py

ascibisz commented 1 year ago

Looks good to me!

toloudis commented 1 year ago

I see broken tests.. floating point comparisons.. and combined with the int casting in the code change, I'm not sure what happened. But the precision for the float comparisons seems way too high. Usually in a unit test you would never test for equality with floating point numbers that possibly had math run on them, but use an epsilon.

mogres commented 1 year ago

I see broken tests.. floating point comparisons.. and combined with the int casting in the code change, I'm not sure what happened. But the precision for the float comparisons seems way too high. Usually in a unit test you would never test for equality with floating point numbers that possibly had math run on them, but use an epsilon.

Yup, looks like very small floating point errors are causing the tests to fail in an OS-dependent way. I'm not sure what the best way to deal with this is, would be happy to get some input. Any thoughts @ascibisz?

ShrimpCryptid commented 1 year ago

Looked into the bug a bit, currently there's an equality assertion between two tuples containing a dictionary and a float array in a few tests. Pytest does have an approx function that handles float array comparison with an epsilon (https://docs.pytest.org/en/7.1.x/reference/reference.html#pytest-approx) that might work if we compare the elements of the tuple?

Something like:

# test_cellpack_convert.py, 156
assert expected_bundleData_data[0] == bundleData["data"][0] && expected_bundleData_data[1] == pytest.approx(bundleData["data"][1]

Also relevant stackoverflow thread if this is happening elsewhere: https://stackoverflow.com/questions/56046524/check-if-python-dictionaries-are-equal-allowing-small-difference-for-floats

mogres commented 1 year ago

Looked into the bug a bit, currently there's an equality assertion between two tuples containing a dictionary and a float array in a few tests. Pytest does have an approx function that handles float array comparison with an epsilon (https://docs.pytest.org/en/7.1.x/reference/reference.html#pytest-approx) that might work if we compare the elements of the tuple?

Something like:

# test_cellpack_convert.py, 156
assert expected_bundleData_data[0] == bundleData["data"][0] && expected_bundleData_data[1] == pytest.approx(bundleData["data"][1]

Also relevant stackoverflow thread if this is happening elsewhere: https://stackoverflow.com/questions/56046524/check-if-python-dictionaries-are-equal-allowing-small-difference-for-floats

I'm trying out a fix using numpy isclose which might work. Will keep you posted!

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (b2ab7fe) 92.55% compared to head (14b17a1) 92.56%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #155 +/- ## ======================================= Coverage 92.55% 92.56% ======================================= Files 101 101 Lines 4542 4546 +4 ======================================= + Hits 4204 4208 +4 Misses 338 338 ``` | [Files Changed](https://app.codecov.io/gh/simularium/simulariumio/pull/155?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [simulariumio/data\_objects/trajectory\_data.py](https://app.codecov.io/gh/simularium/simulariumio/pull/155?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c2ltdWxhcml1bWlvL2RhdGFfb2JqZWN0cy90cmFqZWN0b3J5X2RhdGEucHk=) | `96.87% <100.00%> (+0.15%)` | :arrow_up: | | [...riumio/tests/converters/test\_cellpack\_converter.py](https://app.codecov.io/gh/simularium/simulariumio/pull/155?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c2ltdWxhcml1bWlvL3Rlc3RzL2NvbnZlcnRlcnMvdGVzdF9jZWxscGFja19jb252ZXJ0ZXIucHk=) | `100.00% <100.00%> (ø)` | | | [...ariumio/tests/converters/test\_cytosim\_converter.py](https://app.codecov.io/gh/simularium/simulariumio/pull/155?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c2ltdWxhcml1bWlvL3Rlc3RzL2NvbnZlcnRlcnMvdGVzdF9jeXRvc2ltX2NvbnZlcnRlci5weQ==) | `100.00% <100.00%> (ø)` | | | [...ulariumio/tests/converters/test\_mcell\_converter.py](https://app.codecov.io/gh/simularium/simulariumio/pull/155?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c2ltdWxhcml1bWlvL3Rlc3RzL2NvbnZlcnRlcnMvdGVzdF9tY2VsbF9jb252ZXJ0ZXIucHk=) | `100.00% <100.00%> (ø)` | | | [simulariumio/tests/converters/test\_md\_converter.py](https://app.codecov.io/gh/simularium/simulariumio/pull/155?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c2ltdWxhcml1bWlvL3Rlc3RzL2NvbnZlcnRlcnMvdGVzdF9tZF9jb252ZXJ0ZXIucHk=) | `100.00% <100.00%> (ø)` | | | [...lariumio/tests/converters/test\_medyan\_converter.py](https://app.codecov.io/gh/simularium/simulariumio/pull/155?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c2ltdWxhcml1bWlvL3Rlc3RzL2NvbnZlcnRlcnMvdGVzdF9tZWR5YW5fY29udmVydGVyLnB5) | `100.00% <100.00%> (ø)` | | | [...lariumio/tests/converters/test\_readdy\_converter.py](https://app.codecov.io/gh/simularium/simulariumio/pull/155?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c2ltdWxhcml1bWlvL3Rlc3RzL2NvbnZlcnRlcnMvdGVzdF9yZWFkZHlfY29udmVydGVyLnB5) | `100.00% <100.00%> (ø)` | | | [...ariumio/tests/converters/test\_smoldyn\_converter.py](https://app.codecov.io/gh/simularium/simulariumio/pull/155?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c2ltdWxhcml1bWlvL3Rlc3RzL2NvbnZlcnRlcnMvdGVzdF9zbW9sZHluX2NvbnZlcnRlci5weQ==) | `100.00% <100.00%> (ø)` | | | [...mio/tests/converters/test\_springsalad\_converter.py](https://app.codecov.io/gh/simularium/simulariumio/pull/155?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c2ltdWxhcml1bWlvL3Rlc3RzL2NvbnZlcnRlcnMvdGVzdF9zcHJpbmdzYWxhZF9jb252ZXJ0ZXIucHk=) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.