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

tests: disable test_run_modules::test_grainstats #1100

Conversation

ns-rse
Copy link
Collaborator

@ns-rse ns-rse commented Mar 4, 2025

The refactoring to use classes for objects rather than dictionaries breaks the topostats grainstats entry point that
was introduced with #1094.

Previously the dictionary item image=topostats_object["image"] was passed into processing.run_grainstats() when called from
processing.process_grainstats(). The refactoring requires that image_grain_crop=image_grain_crop, an object of the new type
ImageGrainCrops, is now passed to grainstats.

This isn't currently possible though because AFMReader loads .topostats objects and returns a dictionary and whilst
the refactoring does save the new image_grain_crop (/ImageGrainCrops) the loading does not currently re-create
these structures.

For now I have disabled the test of the entry point. Once this refactoring has been merged we will have to...

  • Make TopoStats a dependency of AFMReader (somewhat wary of this as it may cause headaches further down the line
    but for now we'll go with it!).
  • Modify AFMReader.topostats.load_topostats() to modify the data["grain_tensors"]["above"] and
    data["grain_tensors"]["above"] so that they are of class ImageGrainCrops (and the associated nested classes).
  • Once that is done we can then pass the loaded data["grain_tensors"] to processing.run_grainstats() (this may
    require reconstructing to be the same as image_grain_crop, not sure at the moment!)

EDIT : Looking at this further we have a challenge as currently only the image_grain_crops.[above|below].full_mask_tensors are added to the topostats_object which means that when we load .topostats object it is not possible to create the ImageGrainCrops without first re-running the grain detection. This in turn means its not possible to have entry points for grainstats and all subsequent processing stages until we inlcude this data, at least in raw-format, when we save .topostats objects so that AFMReader can recreate them on loading.

I've noted this and more thoughts down in AFMReader #121.

The refactoring to use classes for objects rather than dictionaries breaks the `topostats grainstats` entry point that
was introduced with #1094.

Previously the dictionary item `image=topostats_object["image"]` was passed into `processing.run_grainstats()` when called from
`processing.process_grainstats()`. The refactoring requires that `image_grain_crop=image_grain_crop`, an object of the new type
`ImageGrainCrops`, is now passed to `grainstats`.

This isn't currently possible though because `AFMReader` loads `.topostats` objects and returns a dictionary and whilst
the refactoring does save the new `image_grain_crop` (/`ImageGrainCrops`) the loading does _not_ currently re-create
these structures.

For now I have disabled the test of the entry point. Once this refactoring has been merged we will have to...

- Make `TopoStats` a dependency of `AFMReader` (somewhat wary of this as it may cause headaches further down the line
  but for now we'll go with it!).
- Modify `AFMReader.topostats.load_topostats()` to modify the `data["grain_tensors"]["above"]` and
  `data["grain_tensors"]["above"]` so that they are of class `ImageGrainCrops` (and the associated nested classes).
- Once that is done we can then pass the loaded `data["grain_tensors"]` to `processing.run_grainstats()` (this may
require reconstructing to be the same as `image_grain_crop`, not sure at the moment!)
@ns-rse ns-rse requested a review from SylviaWhittle March 4, 2025 10:55
@ns-rse ns-rse merged commit d7eda49 into SylviaWhittle/grain_restructure Mar 5, 2025
10 of 19 checks passed
@ns-rse ns-rse deleted the ns-rse/disable-run_grainstats-entry-point-test branch March 5, 2025 12:20
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.

2 participants