AFM-SPM / TopoStats

An AFM image analysis program to batch process data and obtain statistics from images
https://afm-spm.github.io/TopoStats/
GNU Lesser General Public License v3.0
60 stars 11 forks source link

Added average grain width measurement to disordered tracing #999

Open tcatley opened 1 week ago

tcatley commented 1 week ago

Added function calculate_dna_width into class disorderedTrace which uses the smoothed mask and the pruned trace to calculate the average width of the mask using a distance transform. Outputs a float value of the width in metres to the all_statistics.csv.

ns-rse commented 1 week ago

Thanks for the quick update/fixes @tcatley almost there but your second commit #09e10d1 change the column name to grain_width_mean whilst your first commit updated the regression test files with grain_width as the new column.

Thus you need to run the regeneration of the regression test files again. We can avoid duplicating commit messages by ammending and force pushing...

pytest --regtest-reset tests/test_processing.py
git add -u
git commit --amend
git push --force
ns-rse commented 1 week ago

Hi @tcatley,

Thanks for addressing the changes. Unfortunately the tests still fail, but in a different location (this is progress!!! :grin: ).

Here its tests/tracing/test_disordered_tracing.py::test_trace_image_disordered that fails because again we have a dataframe with new columns.

Unfortunately these checks are made outside of the pytest-regtest frame work so we can't just run pytest --regtest-reset tests/tracing/test_disordered_tracing.py::test_trace_image_disordered. Rather we have to uncomment a section of code that writes the target files to disk, run the test to update the target files and include those. This is a bit more involved and for that reason I'll undertake the changes on a separate branch which we can merge back into this branch once I've fixed them.

tcatley commented 1 week ago

Great, thanks @ns-rse! Let me know if you need anything else from me

ns-rse commented 1 week ago

It was a bit more involved than I thought @tcatley and involved another set of regression tests being updated and rebasing onto existing branches that are out for pull requests (#995) which fix the tests in light of a newer version of topoly.

1001 looks big but that is a consequence of "rebasing" which brings the changes from the other branches into yours.

Once #1001 is merged into this we should wait for #995 to be merged to main first (in essence this PR now branches from #995 and my #1001 branches from this #999, for those interested an article I found useful on this is Using stacked pull requests in GitHub - LogRocket Blog).

ns-rse commented 1 week ago

For other reviewers...

I split the failing test into two, one for the nested dictionaries that are loaded from pickles and one for comparing dataframes via pytest-regtest.

I did some more reading on syrupy and think it might be a viable alternative to replace pytest-regtest as tests inherit/have the snapshot function passed in which is used to compare against the returned value of whatever is being tested. These are updated with the command line flag --snapshot-update (akin to --regtest-reset) but objects are stored as serialised objects rather than requiring to be print() and written to file.

MaxGamill-Sheffield commented 6 days ago

@tcatley already had a simple test which I've quickly implemented and expanded slightly. Might need a new reviewer as I've contributed now.