-
Notifications
You must be signed in to change notification settings - Fork 12
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
base: main
Are you sure you want to change the base?
Conversation
…ti class & subgrains
…mask required bool. Tested in debugger.
…2] >= 2, shape[1]==shape[2]
…rainCrops. Locally debugged working
Proposed solution to the data frame issue
|
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. |
Are we aiming to include this refactoring in |
Co-authored-by: Neil Shephard <[email protected]>
f517856
to
cf6b706
Compare
I think this was addressed in #995. Its down to Do you by chance have |
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 😅 |
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
above
andbelow
, each holding aDirectionGrainCrops
object for that direction's grain cropsGrainCropsDirection
crops
andfull_mask_tensor
crops
stores dictionaries ofGrainCrop
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 thecrops
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
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 takeimage
,grain_masks
,pixel_to_nm_scaling
, it now takes justimage_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
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.
docs/configuration.md
docs/usage.md
docs/data_dictionary.md
docs/advanced.md
and new pages it should link to.Optional
topostats/default_config.yaml
If adding options to
topostats/default_config.yaml
please ensure.topostats/validation.py
to ensure entries are valid.topostats/entry_point.py
.