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 topological features into better tracing #898

Conversation

MaxGamill-Sheffield
Copy link
Collaborator

@MaxGamill-Sheffield MaxGamill-Sheffield commented Sep 11, 2024

Adds:

  • branch idx and associated image and funcs
  • topological classifications using the Topoly library
  • writhe classification and associated functions
  • turned grains images rainbow so users could distinguish from background
  • adds many more skan params (min, median, mid values + segment connections)
  • moves get_two_combinations from node stats to tracing funcs as now used in ordered tracing too

Copy link
Collaborator

@ns-rse ns-rse left a comment

Choose a reason for hiding this comment

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

It might be worth merging the #897 first so that the maxgamill-sheffield/topology branch contains the tests and changes before merging into maxgamill-sheffield/800-better-tracing.

pre-commit fails on files that haven't been touched in this change set.

docs/configuration.md

Linting: 14 file(s)
Summary: 7 error(s)
docs/configuration.md:89:97 MD033/no-inline-html Inline HTML [Element: br]
docs/configuration.md:89:124 MD033/no-inline-html Inline HTML [Element: br]
docs/configuration.md:90:97 MD033/no-inline-html Inline HTML [Element: br]
docs/configuration.md:91:97 MD033/no-inline-html Inline HTML [Element: br]
docs/configuration.md:91:116 MD033/no-inline-html Inline HTML [Element: br]
docs/configuration.md:91:143 MD033/no-inline-html Inline HTML [Element: br]
docs/configuration.md:91:166 MD033/no-inline-html Inline HTML [Element: br]

I think these changes come from bd333b4 and the desire to show what the default values for dictionaries are it looks like there is a conflict between markdownlint-cli2 and prettier.

Two solutions as far as I can tell...

  1. Split the dictionary over multiple lines with blank cells in rows.
  2. Add the br element to the list of allowed_elements in .markdownlint-cli2.yaml.
config:
  ...
  html:
    allowed_elements:
      - div
      - br

ruff

The linting with ruff should be straight-forward to resolve.

tests/test_processing.py:285: expected a comma-separated list of codes (e.g., `# noqa: F401, F841`).

Copy & pasta error, need to use the code rather than the pylint error name.

tests/test_processing.py:509:39: F821 Undefined name `run_dnatracing`

The processing.run_dnatracing() function has been removed for some reason which is why this is complaining (can't yet find the commit). Will need an equivalent for the refactored code.

tests/tracing/test_dnatracing_multigrain.py:174:9: F821 Undefined name `trace_image`

Perhaps renamed to dnatrace_image() ? Not found the commit that changed that yet.

tests/tracing/test_dnatracing_multigrain.py:425:15: F821 Undefined name `trace_image`

As above.

topostats/tracing/nodestats.py:1485:9: C901 `average_height_trace` is too complex (11 > 10)
Found 4 errors.

Could disable this check with

# noqa: C901

numpydoc

These too should be straight-forward to resolve.

+---------------------------+------------------------------------------+---------+----------------------------------------------------+
| file                      | item                                     | check   | description                                        |
+===========================+==========================================+=========+====================================================+
| topostats/plotting.py:480 | plotting.plot_crossing_linetrace_halfmax | GL01    | Docstring text (summary) should start in the line  |
|                           |                                          |         | immediately after the opening quotes (not in the   |
|                           |                                          |         | same line, or leaving a blank line in between)     |
+---------------------------+------------------------------------------+---------+----------------------------------------------------+
| topostats/plotting.py:480 | plotting.plot_crossing_linetrace_halfmax | SS05    | Summary must start with infinitive verb, not third |
|                           |                                          |         | person (e.g. use "Generate" instead of             |
|                           |                                          |         | "Generates")                                       |
+---------------------------+------------------------------------------+---------+----------------------------------------------------+
| topostats/plotting.py:480 | plotting.plot_crossing_linetrace_halfmax | PR01    | Parameters {'branch_stats_dict', 'mask_cmap',      |
|                           |                                          |         | 'title'} not documented                            |
+---------------------------+------------------------------------------+---------+----------------------------------------------------+
| topostats/plotting.py:480 | plotting.plot_crossing_linetrace_halfmax | RT01    | No Returns section found                           |
+---------------------------+------------------------------------------+---------+----------------------------------------------------+

These come from two commits

I think the following might solve these (can't make suggestion as not in this PR)...

    """
    Plot the heightmap lines traces of the branches found in the 'branch_stats' dictionary, and their meetings.

    Parameters
    ----------
    branch_stats_dict : dict
        Dictionary containing branch height, distance and fwhm info.
    mask_cmap : matplotlib.colors.Colormap
        Colormap for plotting.
    title : str
        Title for the plot.

    Returns
    -------
    fig, ax
       Matplotlib fig and ax objects.
    """

@ns-rse
Copy link
Collaborator

ns-rse commented Sep 11, 2024

I've addressed most of the above in a separate branch which I will merge into maxgamill-sheffield/800-better-tracing. The renaming/refactoring of dnatracing.trace_image() will take a while though so will be addressed as a separate task.

@ns-rse
Copy link
Collaborator

ns-rse commented Sep 12, 2024

I've addressed most of the above in a separate branch which I will merge into maxgamill-sheffield/800-better-tracing. The renaming/refactoring of dnatracing.trace_image() will take a while though so will be addressed as a separate task.

Further work required this is more involved, see #899

@MaxGamill-Sheffield
Copy link
Collaborator Author

MaxGamill-Sheffield commented Sep 18, 2024

Hey @ns-rse , just seen your comments and want to double check what is required for this to merge:

  1. Add Sylvias testing suite from Add basic smoke tests for topology branch #897
  2. Resolve pre-commit errors: (Done in chore: fix linting errors #900 ?)
  • markdown <br>'s
  • ruff complaints
  • plot_crossing_linetrace_halfmax docstrings
  1. Fix merge conflicts (which I will do now)

Also the merge conflicts in process_scan relate to the changing of grainstats_additions_df to splining_stats and similarly for the per molecule statistics. I might further change these into splining_grainstats and splining_molstats as I think it's clearer what stats each refers to

@ns-rse
Copy link
Collaborator

ns-rse commented Sep 18, 2024

  1. Not sure if you need to merge those into this although there may be issues if you've changed things and it breaks the tests @SylviaWhittle has written.
  2. Yes I've addressed those.
  3. Sorry for the merge conflicts, it's almost unavoidable when three people are working on the same areas of code.

Feel free to go with more meaningful names, I just didn't feel grainstats_additions_df gave much information as to what was being added.

@MaxGamill-Sheffield
Copy link
Collaborator Author

@SylviaWhittle I think the merge conflicts in tests_splining are to merge both, or one is outdated, as they both consider he whole file. Please could you take a look at which one is to be merged and resolve this?

@SylviaWhittle
Copy link
Collaborator

@SylviaWhittle I think the merge conflicts in tests_splining are to merge both, or one is outdated, as they both consider he whole file. Please could you take a look at which one is to be merged and resolve this?

Yeah defo merge both I think

@ns-rse
Copy link
Collaborator

ns-rse commented Oct 2, 2024

For the pyproject.toml conflict would it be possible to order the dependencies alphabetically please (both now and in the future rather than appending to the list). It makes it much easier to read and find what dependencies there are (I corrected this the other day in something).

AFMReader will also need bumping to the top of the list.

@SylviaWhittle
Copy link
Collaborator

SylviaWhittle commented Oct 2, 2024

  • Alphabetised dependencies
  • Resolved merge conflict in test_splines.py

@MaxGamill-Sheffield
Copy link
Collaborator Author

Comments have been addressed so is this ready to go into [maxgamill-sheffield/800-better-tracing]?

@SylviaWhittle
Copy link
Collaborator

Just fixed all the tracing tests 👍

@llwiggins
Copy link
Collaborator

This all looks good to me and I'd be happy for it to be merged, I tested functionality out on a set of varied molecules and all were handled correctly! 👍

@MaxGamill-Sheffield MaxGamill-Sheffield merged commit acf36bc into maxgamill-sheffield/800-better-tracing Oct 7, 2024
2 checks passed
@MaxGamill-Sheffield MaxGamill-Sheffield deleted the maxgamill-sheffield/topology branch October 7, 2024 15:40
@ns-rse
Copy link
Collaborator

ns-rse commented Oct 8, 2024

I tested functionality out on a set of varied molecules and all were handled correctly! 👍

Could these perhaps be used as part of the test suite? It could be as simple as additional regression tests that check the Matplotlib images I suspect are being manually reviewed in a notebook to make the decision that they are being handled correctly, but it would give us some reassurance in the future if/when any of the code is refactored that things aren't broken, or if they do change that we take time to consider if the change is intended.

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