Refactor ReaDDy processing and all visualization #62
Conversation
…line into feature/polymer_twist
jessicasyu
left a comment
jessicasyu
left a comment
There was a problem hiding this comment.
Looks great! Mostly minor things. The only major issue is with the Cytosim simularium trajectories, where the linkers seem to be off.
There's also some missing docstrings, but we can add those in a separate PR so this one doesn't keep getting bigger ;)
subcell_pipeline/analysis/dimensionality_reduction/fiber_data.py
Outdated
Show resolved
Hide resolved
subcell_pipeline/analysis/tomography_data/_analyze_actin_cme_tomography_data.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for making the changes, the visualization updates are very helpful! The PR overall looks good to me. I was able to run the visualization notebooks and generate the simularium trajectories but ran into a couple of issues along the way.
-
In the individual readdy and cytosim visualization notebooks, I ran into an error due to the missing
h5pypackage. After installing it usingpdm add h5pythe notebooks ran as expected. -
I also ran into the issue Jess mentioned regarding the linkers in Cytosim trajectories. They seem to be offset from the main fiber.
subcell_pipeline/analysis/dimensionality_reduction/fiber_data.py
Outdated
Show resolved
Hide resolved
subcell_pipeline/simulation/readdy/_process_readdy_compression_simulations.py
Outdated
Show resolved
Hide resolved
|
@jessicasyu @mogres I think I addressed everything, the Cytosim issue depends on this PR getting merged. I tried fixing the mypy error in the analysis module without success, would be great if someone knows how to fix that. |
jessicasyu
left a comment
jessicasyu
left a comment
There was a problem hiding this comment.
Looks great! Just a couple more things but good to merge after that. I can take a final pass through everything once we get the PRs all merged.
| run: | | ||
| pdm run pytest --cov --cov-report html | ||
| rm htmlcov/.gitignore | ||
There was a problem hiding this comment.
I would keep the build workflow as is (it'll break the coverage badge otherwise). It just won't find any tests to run, which I think is fine.
There was a problem hiding this comment.
it breaks the build otherwise. We can add one test that passes, or maybe the coverage badge should be removed since we know it's zero, unfortunately
There was a problem hiding this comment.
I can refactor the existing test so it passes? It doesn't need to be in this PR
There was a problem hiding this comment.
can I leave it like this for this PR and you can pull this back in with the fixed test?
|
@blairlyons @mogres I believe that last mypy issues is fixed in #61 |
Time Estimate or Size
x-large (sorry!)
Problem
Visualization and post processing scripts were inefficiently organized and out of date.
Solution
I refactored code and tested all of the notebooks in simulation.readdy and visualization modules.
Type of change
Please delete options that are not relevant.
Keyfiles:
Main changes are in simulation.readdy and visualization modules, but I had to make some changes to analysis and other places to support these.