Introduce dataclasses and remodel workflows to use these#1284
Merged
Conversation
- [X] `LoadScans` - [ ] `Filters` - [ ] `Grains` - [ ] `GrainStats` - [ ] `DisorderedTracing` - [ ] `NodeStats` - [ ] `OrderedTracing` - [ ] `Splining`
Switches `Filters()` class over to using `TopoStats` class objects as input. Tests directly on `Filters()` are updated, but integration tests (i.e. of how this impacts on `run_modules.py` and `processing.py`) have _not_ been included in this commit as they also require updating the other classes (`Grains` / `DisorderedTracing` / `NodeStats` / `OrderedTracing` / `Splining`)
The `Grains` class now works with `TopoStats` classes, however...because `GrainCrops` was used in `TopoStats` and this work meant `TopoStats` was used by `Grains` we introduced a circular dependency which Python, reasonably, complains about. The solution has been to move the class definitions to their own modules `topostats.classes`, but that wasn't without some issues since there are static methods of the `Grains` class that were used _within_ `GrainCrop`. For now these have been moved to the `utils` module and I've started writing tests for them (as they didn't appear to have any). As a consequence this commit has a lot of things moving around which _will_ make it a pain to review, but hopefully this will be worth it. For now the whole test suite does _not_ pass all tests because the integration tests where the pipeline is run end-to-end fails. No attempt has been made to correct this yet because ultimately we would like to simply update the `TopoStats` objects and pass them around and that will only be addressed once each processing step/class has been refactored to work with these. Subsequent modules should be a little easier to refactor now that the circular dependencies have been broken.
Switches `GrainStats` to take the `TopoStats` object as an argument and extract the `ImageGrainCrops.GrainCropDirection.crops` (be that `above` or `below`) and calculates the statistics from the returned dictionary. Tests are updated and passed for this module alone, integration tests still fail and will be addressed after all modules are updated.
Managed to omit the `topostats/classes.py` module in a previous commit and noticed some errant `print()` statements in the `GrainStats` refactor so tided those up. Would have use `git commit --fixup` and added these to previous commits but have resolved a merge conflict and didn't want to go round in circles hence making a new commit.
Updates the `DisorderedTracing` class to use `TopoStats` objects. - Removes `.pkl` that are loaded for regression testing in `test/traces/test_disorder_tracing.py::test_trace_image_disordered()` in favour of using [syrupy](https://syrupy-project.github.io/syrupy/) and in doing so closes #1143. Fixes disordered tracing tests, these were broken (by me!) because I had misunderstood the structure of disordered tracing. The whole `TopoStats` object, which contains `GrainCrop` us passed into `disordered_tracing.trace_image_disordered()` function. This handles looping over the various `GrainCrop` and there is currently no need to attempt passing `GrainCrop` into the `disordered_tracing.disordered_trace_grain()` function (which handles instantiating and running the methods associated with `disorderedTrace`). Other things... - Added an as yet unused `GrainCrop` fixture - Updated `test_trace_image_disordered_dataframes()` to use distinct, parameterised, `filename` - Switched `test_trace_image_disordered_dataframes()` to syrupy from regtest - `test_smooth_mask()` - Needed to setup a fixture to take a `GrainCrop` as an argument but this attribute isn't used, - instead the `smooth_mask()` method takes a boolean mask (as int) to perform smoothing, thus we pass that in directly after having instantiated an instance of `disorderedTracing` with the `GrainCrop` to make the method available. - `get_local_pixels_binary()` - A simple test that the surrounding local pixels of a given coordinate are returned as well as testing that `IndexError` is raised if the coordinates are on the edge of the image.
Its cumbersome to have `.npy` / `.pkl` files loaded to restore the targets against which tests are performed because should these need updating users have to manually uncomment code-chunks to save new versions, add the comments back in and then re-run the tests. Fortunately this sort of problem has already been addressed and this PR switches tests that used this strategy to use the [syrupy](https://github.com/syrupy-project/syrupy/) package. The one short coming that is has is that it doesn't natively support Pandas DataFrames or Numpy Arrays, to work around this these are converted to string for "snapshotting". Now when tests need updating then as with `pytest-regtest` that we use to compare dataframes its simply a case of use the `--snapshot-update` flag to update the files. It was felt this was required prior to undertaking refactoring of code to use `TopoStats` objects in case there are changes in the way objects are stored.
Adds two new dataclasses.. - `MatchedBranches` - `Nodes` ...for storing data and attributes related to nodestats rather than the existing `TypedDicts`. - Resurected some resources and moved to `tests/resources/tracing/` which updated... - `tests/tracing/test_disordered_tracing.py` - `tests/tracing/test_ordered_tracing.py` - `tests/tracing/test_splining.py` Need to work out where everything is going to be stored and how to pass disordered_trace data from the `GrainCrop` into `nodestats_image` but typically it's a bit messy. Also need to work out why the `tests/tracing/test_disordered_tracing.py` fails in a few places, I thought I'd finished that work off.
- Move things around in resources so they are better organised. - Disable parallel testing, its a pain when developing and running individual tests, too much setup time. Need to add a bunch of attributes from disordered tracing to `GrainCrop` objects so that they are available.... `disordered_tracing_direction_data` which is a dictionary. Whether this should get passed into the `NodeStats` class I'm unsure, that adds a lot of work passing things around and I would like to get the basic idea of working with GrainCrop objects working and only then dig really deep into replacing things. To which end can probably undo some of the changes in this PR that are made to the nodeStats class as it is `notestats_image()` which instantiates the class with the correct things.
We need to pass around the results of `trace_image_disordered()`, currently this is done by returning dictionaries but we are moving away from that to using `dataclass` so we define a new dataclass for `DisorderedTrace` and we add that as an attribute to the `GrainCrop` class so that we can store the disordered tracing within the `GrainCrop`.
Just starting to get NodeStats(graincrop) working which takes in a `GrainCrop`. These now have the attribute `disordered_trace` which contains a `DisorderedTrace` dataclass containing the `images` (`skeleton` `smoothed_mask`) and other things that were previously in dictionaries.
- Needed to have configuration options for more sections available so added a bunch of fixtures that split these out of the `topostats/default_config.yaml`
I messed up correcting a merge conflict when rebasing so am putting the required `log_topostats_version() back in and will add this commit to `.git-blame-ignore-revs`
- Fixes `processing.run_filters()` and tests to use the TopoStats class. - Adds revision to ignore commit that fixed a bodged rebase - Some tpyos in docstrings of class definitions - Tpyo in `TRACING_RESOURCES` for disordered tracing
- Implements a regression test for `processing.run_disordered_tracing()`. - Checks results are attributes of `GrainCrop` for `minicircle_small`.
Moves closer towards using `TopoStats` class throughout the `processing` module. - Passes `topostats_object: TopoStats` into the various `run_<stage>` functions. - Switches all logging to use the attributes of this class.
- Introduces [pytest-profiling](https://pypi.org/project/pytest-profiling/) as a test dependency so we can profile tests. Introduced because `nodestats` was taking a looooong time to run and its because of long calls to `networkx` that are required to get edges/angles. - Adds `catenane_topostats` and `minicircle_small_topostats` fixtures used in `test_run_nodestats()`. - Tests `run_nodestats`, another step in the right direction of modularising and adding entry points. Note that the `catenane` image has 41 nodes which is one of the reason tests take so long! - Corrects asssertions in `test_run_grains()` to be madea against `topostats_object` attributes rather than pulling out and assigning to `imagegraincrops`. - Rounds out the `Nodes` class with documentation and attributes. - Switches to assessing whether disordered tracing worked by comparing the shape of the dataframe to `(0, 0)` which is the shape of an empty dataframe. Previously this test was done against `if disordered_trace_grainstats is not None` but as the following shows a `pd.DataFrame()` can't be used for truthiness as is normally the case in Python as an empty dataframe is "something" so the test wasn't doing what was expected. ``` pd.DataFrame() is None False pd.DataFrame is not None True ``` It is worth noting that there are some Warnings raised, these were noticed when testing for equality of Nodestats and I've not got the time to investigate these fully, comments have been left in place so we can address in the future and I'll make an issue for these too.
- introduces `ordered_trace` as an attribute to `GrainCrop` class.
- corrects test of equality for skeleton attribute of `GrainCrop`.
- introduce `OrderedTrace` class with attributes for...
- `ordered_trace_data`
- `n_molecules`
- `tracing_stats`
- `grain_mol_stats` - a dictionary of `Molecule`
- `pixel_to_nm_sacling`
- `images`
- `error`
- custom `__eq__` method that checks dictionary of images for equality
- introduce `Molecule` class with attributes
- `circular`
- `topology`
- `ordered_coords`
- `heights`
- `distances`
- test for `processing.run_ordered_tracing()` along with two `.topostats` files in `tests/resources/tracing/ordered_tracing/{catenane,minicircle}_post_nodestats.`
- updates `save_topostats_file()` to work with `TopoStats` boject
- remove errant `print()` from `TopoStats` class
- Switches `save_topostats_file` to work with classes
Required because loading `.topostats` objects from HDF5 AFMReader returns dictionaries. This is ok and I think for now we should not change this as it makes AFMReader very general and of use to others, but internally when we are switching to `TopoStats` classes for all the processing each entry point that loads a `.topostats` file requires a `TopoStats` object so we _have_ to convert these on loading.
- `padding` should be `int()` but was being read as `np.float64()` - mistakenly always tried to set `crop["skeleton"]` even if its not present (in which case it should be `None`).
Add `_to_dict()` methods to each of the following classes... - `MatchedBranch` - `Molecule` - `Node` - `OrderedTrace` ...and ensures these are written to HDF5. Adds dummy objects to `tests/contest.py` and tests the methods work via `tests/test_classes.py`. Currently the types of many of these are _wrong_ because I don't know what they actually represent, that doesn't really matter for the testing though which uses dictionary comprehension and handles any type. Key is that the `GrainCrop.grain_crop_to_dict()` method now works with all of the additional attributes so we can write the full `TopoStats` object to HDF5 which is required for on-going test development of the remaining `OrderedTrace`, `Splining` and `Curvature` so we can write intermediary `.topostats` objects which we can load for tests (instead of running the whole processing pipeline from the start). This is however also **vital** to the additional entry-points (aka "swiss-army knife") work so we can write `.topostats` objects with all of the data upto a given point and load it in the future (previous commit e731084 added the necessary `dict_to_topostats()` function for converting the HDF5-based dictionaries to `TopoStats` objects).
Successes... - Don't attempt to order traces that do not have a disordered trace - `OrderedTrace` class with attributes and methods - `MatchedBranch` class Very messy at the moment, some thoughts... - noticing a number of places where vectorisation could be used instead of loops and some nesting that seems redundant. This won't be addressed in this PR but should be addressed in the future - Dictionaries aren't currently mapped to the classes and their structure, many attributes are themselves dictionaries. - 2025-10-09 - Currently need to get ordered_branches passing around correctly, they are meant to be attributes of `MatchedBranch`. - `tests/resources/tracing/ordered_tracing/catenane_post_nodestats.topostats` is currently 304.4MB which is too big, - need to do something about this. It has been renamed for now to `catenane_post_nodestats_20251013.topostats` because of a conflict when rebasing. Working on making it so we can pickle objects (have added `__getstate__` and `__setstate__` to all classes see next commit)
- adds `thresholds` and `threshold_method` properties to `GrainCrop` class - adds `config` and `full_mask_tensor` properties to `TopoStats` class - updates tests in light of these changes - correct minor tpyo in `default_config.yaml` The main things that this adds though is `__getstate__`/`__setstate__` methods for each of the classes. The reason for doing so is because classes that have `@property` objects associated with them can't be pickled and so they need explicit conversion to dictionaries. See... - [here](https://stackoverflow.com/a/1939384/1444043) - [Handling stateful objects](https://docs.python.org/3/library/pickle.html#pickle-state) Unfortunately this still fails... ``` from pathlib import Path import pickle as pkl from topostats.classes import TopoStats OUTDIR = Path.cwd() OUTFILE = OUTDIR / "empty.topostats" empty_topostats = TopoStats(img_path = None) with OUTFILE.open(mode="wb") as f: pkl.dump(empty_topostats, f) TypeError Traceback (most recent call last) Cell In[905], line 2 1 with OUTFILE.open(mode="wb") as f: ----> 2 pkl.dump(empty_topostats, f) TypeError: cannot pickle 'property' object empty_topostats.__getstate__() {'_image_grain_crops': <property at 0x7fb40c81e0c0>, '_filename': <property at 0x7fb40c81d170>, '_pixel_to_nm_scaling': <property at 0x7fb40c81f880>, '_img_path': PosixPath('/home/neil/work/git/hub/AFM-SPM/TopoStats/tmp'), '_image': <property at 0x7fb39ce731a0>, '_image_original': <property at 0x7fb39ce71e40>, '_full_mask_tensor': <property at 0x7fb39ce72980>, '_topostats_version': <property at 0x7fb39ce71d00>, '_config': <property at 0x7fb39ce72020>} ``` Everything is _still_ a `property`. This dummy example works fine though... ``` @DataClass class dummy(): var1: int | None = None var2: float | None = None var3: str | None = None var4: list[int] | None = None var5: dict[str, str] | None = None def __getstate__(self): # return {"_var1": self._var1, # "_var2": self._var2, # "_var3": self._var3, # "_var4": self._var4, # "_var5": self._var5,} state = self.__dict__.copy() return state def __setstate__(self, state): # self._var1 = state["_var1"] # self._var2 = state["_var2"] # self._var3 = state["_var3"] # self._var4 = state["_var4"] # self._var5 = state["_var5"] self.__dict__.update(state) @Property def var1(self) -> int: """ Getter for the ``var1`` attribute. Returns ------- int Returns the value of ``var1``. """ return self._var1 @var1.setter def var1(self, value: int) -> None: """ Setter for the ``var1`` attribute. Parameters ---------- value : int Value to set for ``var1``. """ self._var1 = value @Property def var2(self) -> float: """ Getter for the ``var2`` attribute. Returns ------- float Returns the value of ``var2``. """ return self._var2 @var2.setter def var2(self, value: float) -> None: """ Setter for the ``var2`` attribute. Parameters ---------- value : float Value to set for ``var2``. """ self._var2 = value @Property def var3(self) -> str: """ Getter for the ``var3`` attribute. Returns ------- str Returns the value of ``var3``. """ return self._var3 @var3.setter def var3(self, value: str) -> None: """ Setter for the ``var3`` attribute. Parameters ---------- value : str Value to set for ``var3``. """ self._var3 = value @Property def var4(self) -> list[int]: """ Getter for the ``var4`` attribute. Returns ------- list[int] Returns the value of ``var4``. """ return self._var4 @var4.setter def var4(self, value: list[int]) -> None: """ Setter for the ``var4`` attribute. Parameters ---------- value : list[int] Value to set for ``var4``. """ self._var4 = value @Property def var5(self) -> dict[str,str]: """ Getter for the ``var5`` attribute. Returns ------- dict[str,str] Returns the value of ``var5``. """ return self._var5 @var5.setter def var5(self, value: dict[str,str]) -> None: """ Setter for the ``var5`` attribute. Parameters ---------- value : dict[str,str] Value to set for ``var5``. """ self._var5 = value OUTFILE = OUTDIR / "empty.dummy" empty_dummy = dummy() with OUTFILE.open(mode="wb") as f: pkl.dump(empty_dummy, f) ``` ...no error and I don't understand where I/we have gone wrong?!?!?!?! I'm somewhat inclined to move away from `@dataclass` and using `@property` to provide the `setter` / `getter` design pattern and instead use plain classes with attributes.
Moves to [Pydantic dataclasses](https://docs.pydantic.dev/latest/concepts/dataclasses/) for stricter data validation. This means we can pickle `TopoStats` objects which is useful because in the test suite we don't want to run the whole pipeline when we want to test e.g. `Nodestats`. As a consequence we now have pickles which are loaded as `pytest.fixtures` (from `tests/conftest.py`) rather than lines of code within tests themselves that save and modify `.npy`/`.pkl` files. There are therefore three sets of pickles... - `minicircle_small` - `catenanes` - `rep_int` ...at different stages... - `_post_grainstats` - `_post_disordered_tracing` - `_post_nodestats` ...and we will develop additional fixtures for... - `_post_ordered_tracing` - `_post_curvature` - `_post_splining` (optional, not required at the moment as no subsequent processing is done after this) A slight disconnect might arise from how these pickles were created, at the moment it is code in a `.py` file on @ns-rse computer. @ns-rse will look at adding this as an additional script in the repository, but as more work is required its not included at the moment. This now allows me to finish of re-factoring and writing the integration test for `ordered-tracing`.
This function used the last part of the path in `basename` column from the dataframes which is actually the image `<filename>` without the extension to write data to CSVs under `<output_dir>/<filename>/processed/*.csv`. This seemed wrong for two reasons... 1. Its a "per-image" and not "per-folder containing images" CSV file. 2. Per-image output resides under `<output_dir>/processed/<filename>` Instead we now right to the output directory in point 2 above and the function has been renamed `save_image_grainstats()`. Personally I'm not sure this is actually needed. I've tried digging through the commit history to find out why we have this. We can use `git grep "<term>"` to search but if we want to look at the actual code that is committed rather than just search the commit messages (which is the default that is searched by `git grep`) then we have to pass in the `git rev-list --all` although we can narrow the scope down by adding the file we want to search. ``` git grep "def folder_grainstats" $(git rev-list --all -- topostats/io.py) ``` ...which shows it was moved to `topostats/io.py` from `topostats/utils.py` with lots of other references/inclusions (output omitted for brevity). None of this helped much though so I have searched the issues and found #224 which discusses the need for the output to be subsetted so that it can be input to `Plotting.py`. We reworked this to `topostats/plotting.py` that plots summary distributions and I'm not sure there is much need for per-image _or_ per-folder CSV files these days. Being able to load the CSV files into a Notebook and subset the data to plot what is required is a fairly useful and common skill for anyone doing scientific research to have in their skills repertoire and I think we have over-engineered the output for too specific a case in the past and I would propose removing the call to this function at the very least, if not the function and its tests entirely.
| | `unmatched_branch_stats` | `dict[int, UnMatchedBranch]` | Dictionary of unmatched branch statistics. | | ||
| | `node_coords` | `npt.NDArray[np.int32]` | Numpy array of node coordinates. | | ||
| | `confidence` | `np.float64` | Confidence in ???. | | ||
| | `reduced_node_area` | `???` | Reduced node area. | |
Collaborator
There was a problem hiding this comment.
Suggested change
| | `reduced_node_area` | `???` | Reduced node area. | | |
| | `reduced_node_area` | `npt.NDArray[np.float64]` | Reduced node area. | |
Collaborator
|
Re-tested with varying config parameters.
All without issue. |
Collaborator
Author
Yay! Thanks for the thorough testing @SylviaWhittle 🙌 Because this underpins a new release that is required for @SylviaWhittle paper I propose merging this and making a release candidate. Paper submissions can then be made as and when they are ready and reference the full new release (not the release candidate). This gives us some time to allow the release candidate to be battle tested and any bugs/problems addressed. One question I have is what the version should be? Notionally I've been working on it becoming |
SylviaWhittle
approved these changes
Feb 3, 2026
Collaborator
SylviaWhittle
left a comment
SylviaWhittle
left a comment
There was a problem hiding this comment.
I think we can take this to main and understand that it'll be unstable for a few months while we make hot fixes for any issues.
It'll be easier to make PRs against main than to add more commits & review them in this monster of a PR.
Thank you so much Neil.
Collaborator
|
I'm happy for it to be |
This was referenced Feb 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NB - I expect there to be questions and problems with this PR, it needs and would benefit from wide testing on lots
of different samples.
Building on the introduction of dataclasses that @SylviaWhittle begun for
GrainCropsthis pull request rounds that outand implements dataclasses throughout. The top-level
TopoStatsdataclass holds meta-data and image information andthere are classes for...
GrainCropsDisorderedTraceNodeOrderedTraceMatchedBranchMoleculeWe use Pydantic dataclasses which offer data validation on instantiation of classes
and because of the options that have been set when attempting to modify/update values validation is also used. This
ensures the types of data are set/used in each of the classes is always correct and helps minimise edge case problems
when developing in the future.
Direction
After discussion we have removed the concept of processing for
aboveandbelowand in turn removed theImageGrainCropsandGrainCropsDirectionclasses. Bothaboveandbelowcan now have multiple thresholds (so thatdifferent types of molecules can be detected) and so it was felt that the concept/distinction was arbitrary. Thresholds
are available in both of these traditional directions and can have one or more values but we just don't nest the results
by direction. Instead
GrainCrop.thresholdis an attribute which defines the threshold used to detect that particulargrain.
Output
As a consequence some of the output files have changed (as of writing I may have one more of some output fields to
do). Some of the more detailed, grain level plots have moved location to all be in nested directories reflecting the
processing stage they have come from.
Development Implications
As a consequence we no longer pass around dataframes that need things adding to them. Instead all data is stored as
attributes to the different levels of classes. At the end of processing we build dictionaries of the attributes. These
are aggregated into nested dictionaries and are converted to Pandas Dataframes for writing to CSV.
Nomenclature
A big challenge in the refactoring stemmed from ambiguous nomenclature, "skeletons" are used in many stages of tracing
but there are lots of different types of skeleton. This has hopefully been clarified so that it is clear which
skeleton is used at which stage. Further abbrv. where encoutered have been renamed to be more descriptive. They might
have made sense when writing but lack meaning to those new to the code (including future selves!).
Configuration
Where configuration options have changed the corresponding validation has been updated.
Tests
Many tests required updating as a consequence and I think (hope!) I have managed to preserve consistency.
Regression tests
In doing this we have switched from
pytest-regtesttosyrupyfor regression testing of output.I have also attempted to remove a large number of
.pkl's (Python pickles which are seralised Python objects saved todisk) which were used to compare test results against. Some still remain though. The solution to this is noted in
tests/conftest.pybut broadly it is to keep fully processed.topostatsfiles for test images, load these as fixturesand then set the attributes that are to be tested to
Nonefor the tests that perform the calculations. I would haveliked to have done this myself but have run out of time.
Closes
This pull request closes multiple issues. I think it covers the following at the very least but may also close others
not listed below
Closes #1112
Closes #1113
Closes #1114
Closes #1115
Closes #1116
Closes #1117
Closes #1134
Closes #1140
Closes #1152
Closes #1256
Closes #1279
Closes #1282
ToDo
There are still entry points that are required to be written (see #517) but now that we have a consistency between the internal
representation of
TopoStatsobjects and how these are stored in.topostatsHDF5 files (and in turn loaded) theseshould be relatively straight-forward. See the existing examples for
filters/grains/grainstatsfor examples ofhow to do this (adding sub-parsers with relevant options and a function in
topostats.run_modules.pythat usespartial()to run a function defined intopostats.processing.pywhich loads.topostatsfiles and runs the moduleusing the relevant
topostats.processing.run_<module>()).Before submitting a Pull Request please check the following.
docs/configuration.mddocs/usage.mddocs/data_dictionary.mddocs/advanced.mdand new pages it should link to. Includesdocs/advanced/classes.pywhich seeks to explainhow thte dataclasses are structured.
are used in new functions.
Optional
topostats/default_config.yamlIf adding options to
topostats/default_config.yamlplease ensure.topostats/validation.pyto ensure entries are valid.topostats/entry_point.py.