Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds unit-tests for dnatracing methods #593

Merged
merged 5 commits into from
Jun 2, 2023
Merged

Conversation

ns-rse
Copy link
Collaborator

@ns-rse ns-rse commented Jun 1, 2023

Also removes unused functions from dnatracing

In looking to change the processing path and have the unit of processing be a single grain rather than the whole image and negate the need for every subsequent method to have a for x, <something> in emuerate(<list_of_grains>): we need tests in place to make sure things work as expected.

The tests use grain 3 and 9 from minicircle.spm when selecting these 3 was determined to be linear and 9 was determined to be circular. Strangely though when processing these through the tests both are reported as being circular and so the tests aren't quite complete, but they are useful in that they allow the loops to be removed and the logic shifted to process_scan() (these will be in a forthcoming PR).

Also removes unused functions from dnatracing
@ns-rse ns-rse requested a review from Jean-Du June 1, 2023 14:12

plt.plot(coordinates_array[:, 0], coordinates_array[:, 1], "ko")
plt.savefig(f"{savename}_{dna_num}_coordinates.png")

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ns-rse , would it be worth keeping writeCoordinates? I think it's fine to remove writeContourLengths given that we already have contour lengths in the all_statistics.csv spreadsheet, but do we have the coordinates printed out anywhere else? If not, it might be worth keeping it and producing it alongside other future tracing outputs when toggled on?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it as its not used anywhere in the code as far as I can tell.

If writing co-ordinates is something that is required it should be a function within the topostats.io module which handles input/output and imported where required.

As it stands this method also does two things...

  1. Writes a CSV file.
  2. Produces plots.

The later should be separated and may already be covered by functionality that already exists.

I've added the desire to write co-ordinates to CSV to #183 (see comment) since this isn't used and I would write it differently anyway to fit within the directory structure of output, use pathlib rather than os and so forth.

Copy link
Collaborator

@Jean-Du Jean-Du Jun 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it's not used in the main branch, I do use it in my branch. When I merged some of my work into the main back when we were on TopoStats 1.0, I did not enable it as some of my work wasn't complete, but I did intend to keep it. The idea of this method is to produce the coordinates of the traces, so that when other tracing based measurements went wrong, I could always look at the csv files and the plots to check whether the tracing was satisfatory, so as long as the functionalities are covered elsewhere, deleting them here should be fine.

@codecov-commenter
Copy link

codecov-commenter commented Jun 1, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.81 🎉

Comparison is base (e77fb2b) 80.84% compared to head (f1ce13b) 81.65%.

❗ 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
@@            Coverage Diff             @@
##             main     #593      +/-   ##
==========================================
+ Coverage   80.84%   81.65%   +0.81%     
==========================================
  Files          19       19              
  Lines        2814     2824      +10     
==========================================
+ Hits         2275     2306      +31     
+ Misses        539      518      -21     
Impacted Files Coverage Δ
topostats/tracing/dnatracing.py 66.08% <ø> (+2.81%) ⬆️
topostats/grains.py 98.01% <100.00%> (ø)

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@SylviaWhittle SylviaWhittle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very rigorous. I think that catches everything. Thanks for adding the parametrised tests for linear_or_circular.

This looks great to me 👍

@ns-rse ns-rse added this pull request to the merge queue Jun 2, 2023
Merged via the queue into main with commit 9849154 Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants