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

Refactor pipeline to use grain crop dictionaries #1022

Open
wants to merge 145 commits into
base: main
Choose a base branch
from

Conversation

SylviaWhittle
Copy link
Collaborator

@SylviaWhittle SylviaWhittle commented Nov 22, 2024

This is a draft PR and documentation / tests will be added before full PR is made

This PR is huge (sorry)

Main things

This PR is designed to improve how we handle grains in the processing stages of TopoStats, starting at the grain finding stage, up to the disordered tracing stage. In future, this might be extended through the disordered tracing stage and beyond, however I've restricted the scope of this PR for the sake of everyone's sanity. The reason for stopping at disordered tracing is that once disordered tracing returns, all the data is wrapped up in neatly structured dictionaries, by grain and molecule, similar to what I've implemented, so I deemed this similar enough to not bother changing it yet.

The way this PR tries to standardize how we handle grains, is using DataClasses:

  • ImageGrainCrops
    • has two attributes, above and below, each holding a DirectionGrainCrops object for that direction's grain crops
  • GrainCropsDirection
    • two attributes: crops and full_mask_tensor
    • crops stores dictionaries of GrainCrop objects ([int, GrainCrop])
    • full_mask_tensor stores a full sized mask for the image, size is NxNxC where C is the number of classes. This is NOT automatically updated when the crops property is edited, this is because we don't want to update things during a loop. This can be discussed if this is an incorrect decision!
  • GrainCrop
    • Stores various properties about the grain, such as mask, image, bbox padding etc.

This has the benefit of standardizing how we handle grains going forward, as we had previously been rather discordant in the types of data structures that we use in various parts of the codebase.

It also adds a helpful (I hope!!) layer of abstraction to processing functions, for example the run_grainstats function in processing no longer needs to take image, grain_masks, pixel_to_nm_scaling, it now takes just image_grain_crops which contains all the data for each crop.

This of course does come at the cost of increased memory usage as there are duplication of parts of images in the data structures as well as repeatedly listing the pixel_to_nm_scaling factor etc, however I personally find that the benefits here far outweigh the negatives. When working on the harbo-rings project, I found myself naturally extracting all the grains and storing them in a dictionary rather than keeping track of full image masks, I know Max also does this based on how he's handled the tracing code.

disordered_tracing.py

  • Removed prep_arrays. Prep arrays no longer needed, since it made a dictionary of grain crops, but we already have these now with the refactor.

TopoStats Pull Requests

Please provide a descriptive summary of the changes your Pull Request introduces.

The Software Development section of
the Contributing Guidelines may be useful if you are unfamiliar with linting, pre-commit, docstrings and testing.

NB - This header should be replaced with the description but please complete the below checklist or a short
description of why a particular item is not relevant.


Before submitting a Pull Request please check the following.

  • Existing tests pass.
  • Documentation has been updated and builds. Remember to update as required...
    • docs/configuration.md
    • docs/usage.md
    • docs/data_dictionary.md
    • docs/advanced.md and new pages it should link to.
  • Pre-commit checks pass.
  • New functions/methods have typehints and docstrings.
  • New functions/methods have tests which check the intended behaviour is correct.

Optional

topostats/default_config.yaml

If adding options to topostats/default_config.yaml please ensure.

  • There is a comment adjacent to the option explaining what it is and the valid values.
  • A check is made in topostats/validation.py to ensure entries are valid.
  • Add the option to the relevant sub-parser in topostats/entry_point.py.

SylviaWhittle and others added 21 commits November 20, 2024 14:35
@SylviaWhittle
Copy link
Collaborator Author

Proposed solution to the data frame issue

|----------------------------------------------------------------------------------------------------------------------|
|   image   |    direction   |     class         | grain | molecule | ... <grainstats> ... | ... <dnatracingstats> ... |
| mini.spm  |    above       |   dna_only        | 0     | 0        | ... <stats> ...      |          NONE             |
| mini.spm  |    above       |   dna_only        | 0     | 1        | ... <stats> ...      |          NONE             |
| mini.spm  |    above       |   dna_only        | 1     | 0        | ... <stats> ...      |          NONE             |
| mini.spm  |    above       |   dna_only        | 1     | 1        | ... <stats> ...      |          NONE             |
| mini.spm  |    above       |   protein_only    | 0     | 0        | ... <stats> ...      |          NONE             |
| mini.spm  |    above       |   protein_only    | 0     | 1        | ... <stats> ...      |          NONE             |
| mini.spm  |    above       |   protein_only    | 1     | 0        | ... <stats> ...      |          NONE             |
| mini.spm  |    above       |   protein_only    | 1     | 1        | ... <stats> ...      |          NONE             |
| mini.spm  |    above       |   combined_mask   | 0     | 0        | ... <stats> ...      |     ... <stats> ...       |
|----------------------------------------------------------------------------------------------------------------------|

@ns-rse
Copy link
Collaborator

ns-rse commented Dec 3, 2024

Its that or split into separate files.

I'm ambivalent as to the preferred solution as I don't use the output but consideration for end users should be given. Whilst data management, manipulation, summarisation and plotting are, in my view, core skills for researchers these days experience levels vary widely and I don't know what would be easiest.

@ns-rse
Copy link
Collaborator

ns-rse commented Dec 10, 2024

Are we aiming to include this refactoring in v2.3.0 release?

@SylviaWhittle SylviaWhittle force-pushed the SylviaWhittle/grain_restructure branch from f517856 to cf6b706 Compare February 5, 2025 10:28
@SylviaWhittle
Copy link
Collaborator Author

"it works on my machine!" (tests fail on GH-A, but not on my machine, seems to be due to this issue again:

image

@ns-rse
Copy link
Collaborator

ns-rse commented Feb 11, 2025

"it works on my machine!" (tests fail on GH-A, but not on my machine, seems to be due to this issue again:

I think this was addressed in #995.

Its down to topoly versions, the GitHub Action uses topoly-1.0.4 and we can see in the changeset of this pull request that these are reverted in the target CSV tests/resources/tracing/ordered_tracing/catenanes_ordered_tracing_molstats.csv which revert to the older version.

Do you by chance have topoly-1.0.2 on your system/virtualenv? (pip show topoly) If so can you upgrade to topoly-1.0.4 and revert the above mentioned file as I think tests would then pass locally for you and on GitHub Actions.

@SylviaWhittle
Copy link
Collaborator Author

Okay I have ticked all the boxes in the PR guide (that is such a good feature) I believe. I'm absolutely sure I'll have missed big things, but I'll submit for review now.

Reviewers, please do just flag stuff as it comes, don't assume I've done something right and waste time trying to comprehend my silly ways of doing things, I don't want to take up more of your time than is absolutely necessary 👍

Also @ns-rse, IIRC you like to go through a PR commit-by-commit. If so, I would strongly advise against this because I developed this PR in a very broken state to begin with, the early commits won't make sense / reflect the end PR. I am very sorry for how awful this PR is 😅

@SylviaWhittle SylviaWhittle marked this pull request as ready for review February 11, 2025 14:45
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.

3 participants