Removal of above/ below in grain thresholds

[tobyallwood]

Remove above/ below from grain thresholds

Closes #1332
Closes #1333
Closes #1334

This PR simplifies the grain finding thresholds by removing a layer of indentation (above/ below keys). The direction of the threshold is now selected by the sign of the threshold value in the config. For example:

old:

threshold_std_dev:
    below: 10.0
    above: 1.0

new:

threshold_std_dev: [-10.0, 1.0]

---

This also means that area_thresholds has changed to lose the directional indentation layer:

old:

area_thresholds:
  above: [null, null]
  below: [300, 3000]

new:

area_thresholds: [[null, null], [300, 3000]]

The new area_thresholds argument is a 2d list, each sub-list is for the threshold it shares an index number with.

---

Any reference/ use of direction in loops through TopoStats have either been removed or changed into a single loop of all thresholds in the list regardless of their direction. They are referred to via their index in the thresholds list.

---

Before submitting a Pull Request please check the following.

• Existing tests pass.
• Documentation has been updated and builds. Remember to update as required...
  • docs/usage/configuration.md
  • docs/usage/data_dictionary.md
  • docs/usage/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.

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.

[ubdbra001]

I had a quick skim and made some comments on things that jump out at me.

The changes to the docs are minimal, and from looking at them I'm not clear about what values I can include for these config options (Does order matter, how many values can I include, etc.) I'm probably not remembering the context for these options, but I think it would be good to spell them out somewhere.

[ubdbra001]

Just for my understanding:

• Is there a specific order for these (i.e. would [-10, 1] produce the same result as [1, -10])?
• Are only 2 values allowed?
• Should one be negative and one positive?

[tobyallwood]

• There's no specific order but it's important to be aware of the order you chose as in the output .csvs the thresholds are referenced by index.

• No, it can be any amount (except none)

• No, you can have any configuration

I should definitely make this clearer, I will in the docs but would more information be good in the config file too?

[ubdbra001]

If you want to you can, but I don't think it's vital. It's worth bearing in mind that if you have the same info in multiple places and they both have to be manually updated for future changes there is a reasonable chance that one of those places will get overlooked and you will end up with differing information.

I saw you've updated the schema for validation. It would be good to update this to ensure an empty list can't be passed (if I'm interpreting your answer correctly)

[tobyallwood]

Failing tests all seem to be plot image checks, the output image and the expected image have data such as axes ticks, plot titles, legends etc. translated a few pixels over

Edit: this now seems to have fixed itself??

[tobyallwood]

One thing I'm noting here for documentation/ future reference purposes:

In grain_statistics.csv do we want to reference the threshold by its index in the threshold list or by the actual value of the threshold? Or is this field required at all?

Assuming it is required...

If the thresholds are [-1, 1] and a grain is found using -1 (aka below 1), would we want to use

threshold: 0

or

threshold: -1

---

My immediate thought is that the second option seems better but might make std_dev thresholds look confusing to a user as the number saved would not be the number they entered into the config.

On the other hand clear and obvious documentation will be needed if we reference the threshold by index so a threshold: 1 is understood as index 1 rather than the actual threshold value being 1.

Previously the closest thing we had to this value was called direction and held either "above" or "below" but I think it would be more useful to point to a specific threshold seeing as multiple thresholds can be in the same direction

As far as I can see either option will require fairly minimal additions to implement, the threshold index is already stored and used in log messages and the actual threshold value can easily be found with that index value.

[SylviaWhittle]

One thing I'm noting here for documentation/ future reference purposes:

In grain_statistics.csv do we want to reference the threshold by its index in the threshold list or by the actual value of the threshold? Or is this field required at all?

Assuming it is required...

If the thresholds are [-1, 1] and a grain is found using -1 (aka below 1), would we want to use

threshold: 0

or

threshold: -1

My immediate thought is that the second option seems better but might make std_dev thresholds look confusing to a user as the number saved would not be the number they entered into the config.

On the other hand clear and obvious documentation will be needed if we reference the threshold by index so a threshold: 1 is understood as index 1 rather than the actual threshold value being 1.

Previously the closest thing we had to this value was called direction and held either "above" or "below" but I think it would be more useful to point to a specific threshold seeing as multiple thresholds can be in the same direction

As far as I can see either option will require fairly minimal additions to implement, the threshold index is already stored and used in log messages and the actual threshold value can easily be found with that index value.

(gently held opinion) I think we should use index for now.

Perhaps in the future we make the user name the thresholds, ie:

threshold_std_dev
 - "main" : 1.0
 - "protein" : 2.2
 - "holes" : -0.5

[SylviaWhittle]

♡

[SylviaWhittle]

This is great, thank you for redoing this work. I appreciate it.

I also like the sweeping up you've done, keeping our kitchen tidy. :)

[SylviaWhittle]

That's very odd - its failing on the test for unet preprocessing for making bounding boxes square. This code should be very isolated and resilient...

[tobyallwood]

XFAIL tests/test_unet_masking.py::test_make_bounding_box_square[rectangular image with large rectangular box] - square bounding box not possible
FAILED tests/test_processing.py::test_process_scan - AssertionError: assert [+ received] == [- snapshot]

@SylviaWhittle Is it not the snapshot comparison in test_process_scan and test_unet_masking is just an xfail?

[SylviaWhittle]

XFAIL tests/test_unet_masking.py::test_make_bounding_box_square[rectangular image with large rectangular box] - square bounding box not possible
FAILED tests/test_processing.py::test_process_scan - AssertionError: assert [+ received] == [- snapshot]

@SylviaWhittle Is it not the snapshot comparison in test_process_scan and test_unet_masking is just an xfail?

Ah yes. I skimmed too rapidly hah.