AFM-SPM / TopoStats

An AFM image analysis program to batch process data and obtain statistics from images
https://afm-spm.github.io/TopoStats/
GNU Lesser General Public License v3.0
60 stars 11 forks source link

Adding the Better Tracing Suite into Main #932

Closed MaxGamill-Sheffield closed 1 month ago

MaxGamill-Sheffield commented 1 month ago

Closes #800

This aims to add features which split out the DNATracing pipeline into smaller parts (disordered tracing, ordered tracing and splining) while adding individual analyses in each of these to be more modular. It also adds topological functions and analyses into ordered tracing, facilitating the processing of catenated molecules as separate objects, and includes a new module to handle and analyse crossings of DNA segments.

SylviaWhittle commented 1 month ago

Lots of merge conflicts to solve here. Do we want to do this together?

ns-rse commented 1 month ago

As well as the conflicts there are still lots of tests failing which need fixing before this can be merged. The purpose of the tests is to ensure when we do change things, as being done here, we aren't breaking anything. If tests need updating in light of changes then that is fine but we can't just abandon good coding practices.


======================================================================== short test summary info =========================================================================
SKIPPED [1] tests/test_utils.py:678: awaiting test development
SKIPPED [2] tests/tracing/test_dnatracing_single_grain.py:119: Need to correctly prune arrays first.
SKIPPED [2] tests/tracing/test_dnatracing_single_grain.py:151: Need to correctly prune arrays first.
SKIPPED [2] tests/tracing/test_dnatracing_single_grain.py:173: Need to correctly prune arrays first.
SKIPPED [4] tests/tracing/test_dnatracing_single_grain.py:200: Need to correctly prune arrays first.
SKIPPED [2] tests/tracing/test_dnatracing_single_grain.py:224: Need to correctly prune arrays first.
SKIPPED [2] tests/tracing/test_dnatracing_single_grain.py:253: Need to correctly prune arrays first.
SKIPPED [2] tests/tracing/test_dnatracing_single_grain.py:295: Need to correctly prune arrays first.
SKIPPED [2] tests/tracing/test_dnatracing_single_grain.py:317: Need to correctly prune arrays first.
SKIPPED [2] tests/tracing/test_pruning.py: two branches at the end does not have 'nibs' removed by the rm_nibs() function, instead a T-shaped junction remains. This is only removed by calling getSkeleton(method='zhang').get_skeleton() which is done under the prune_all_skeletons() method but this method is is to be removed since looping over all skeletons is outside of the scope/remit of the code and handled in process_scan().
SKIPPED [1] tests/tracing/test_dnatracing_single_grain.py:101: Need to correctly prune arrays first.
SKIPPED [1] tests/tracing/test_pruning.py: unconditional skip
XFAIL tests/tracing/test_pruning.py::test_local_area_sum[3x3 1's; point on left edge] - Point on left edge of image.
XFAIL tests/tracing/test_pruning.py::test_local_area_sum[3x3 1's; point on bottom edge] - Point on bottom edge of image.
XFAIL tests/tracing/test_pruning.py::test_local_area_sum[3x3 non-binary array; point as list] - Array is not binary.
XFAIL tests/tracing/test_pruning.py::test_local_area_sum[3x3 1's; point on corner] - Point on corner of image.
XFAIL tests/tracing/test_pruning.py::test_local_area_sum[3x3 1's; point on top edge] - Point on top edge of image.
XFAIL tests/tracing/test_pruning.py::test_local_area_sum[3x3 1's; point on right edge] - Point on right edge of image.
ERROR tests/measure/test_geometry.py
ERROR tests/test_entry_point.py
ERROR tests/test_plotting.py
ERROR tests/test_processing.py
ERROR tests/test_run_topostats.py
ERROR tests/tracing/test_nodestats.py
ERROR tests/tracing/test_ordered_tracing.py
FAILED tests/tracing/test_dnatracing_methods.py::test_linear_or_circular[figure8] - AttributeError: 'dnaTrace' object has no attribute 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_methods.py::test_linear_or_circular[three_ends] - AttributeError: 'dnaTrace' object has no attribute 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_methods.py::test_linear_or_circular[vertical] - AttributeError: 'dnaTrace' object has no attribute 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_methods.py::test_linear_or_circular[six_ends] - AttributeError: 'dnaTrace' object has no attribute 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_methods.py::test_linear_or_circular[horizontal] - AttributeError: 'dnaTrace' object has no attribute 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_methods.py::test_linear_or_circular[diagonal1] - AttributeError: 'dnaTrace' object has no attribute 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_methods.py::test_get_splined_traces[trace_image0-1.0-True-0.0-expected_spline_image0] - TypeError: dnaTrace.get_splined_traces() missing 2 required positional arguments: 'fitted_trace' and 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_methods.py::test_linear_or_circular[diagonal2] - AttributeError: 'dnaTrace' object has no attribute 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_methods.py::test_get_splined_traces[trace_image1-1.0-True-10.0-expected_spline_image1] - TypeError: dnaTrace.get_splined_traces() missing 2 required positional arguments: 'fitted_trace' and 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_methods.py::test_get_splined_traces[trace_image7-5.0-False-5.0-expected_spline_image7] - TypeError: dnaTrace.get_splined_traces() missing 2 required positional arguments: 'fitted_trace' and 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_methods.py::test_linear_or_circular[diagonal3] - AttributeError: 'dnaTrace' object has no attribute 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_methods.py::test_linear_or_circular[circle] - AttributeError: 'dnaTrace' object has no attribute 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_methods.py::test_linear_or_circular[blob] - AttributeError: 'dnaTrace' object has no attribute 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_methods.py::test_get_splined_traces[trace_image2-10.0-True-0.0-expected_spline_image2] - TypeError: dnaTrace.get_splined_traces() missing 2 required positional arguments: 'fitted_trace' and 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_methods.py::test_get_splined_traces[trace_image3-10.0-True-5.0-expected_spline_image3] - TypeError: dnaTrace.get_splined_traces() missing 2 required positional arguments: 'fitted_trace' and 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_methods.py::test_linear_or_circular[cross] - AttributeError: 'dnaTrace' object has no attribute 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_methods.py::test_linear_or_circular[singl_L] - AttributeError: 'dnaTrace' object has no attribute 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_single_grain.py::test_gaussian_filter[linear] - KeyError: 'dnatracing'
FAILED tests/tracing/test_dnatracing_methods.py::test_get_splined_traces[trace_image4-5.0-False-5.0-expected_spline_image4] - TypeError: dnaTrace.get_splined_traces() missing 2 required positional arguments: 'fitted_trace' and 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_methods.py::test_linear_or_circular[double_L] - AttributeError: 'dnaTrace' object has no attribute 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_methods.py::test_linear_or_circular[diagonal_end_single_L] - AttributeError: 'dnaTrace' object has no attribute 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_methods.py::test_get_splined_traces[trace_image5-4.0-False-5.0-expected_spline_image5] - TypeError: dnaTrace.get_splined_traces() missing 2 required positional arguments: 'fitted_trace' and 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_methods.py::test_linear_or_circular[diagonal_end_straight] - AttributeError: 'dnaTrace' object has no attribute 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_methods.py::test_get_splined_traces[trace_image6-5.0-False-0.0-expected_spline_image6] - TypeError: dnaTrace.get_splined_traces() missing 2 required positional arguments: 'fitted_trace' and 'mol_is_circular'
FAILED tests/tracing/test_dnatracing_single_grain.py::test_gaussian_filter[circular] - KeyError: 'dnatracing'
FAILED tests/tracing/test_dnatracing_single_grain.py::test_get_disordered_trace[linear lee] - KeyError: 'dnatracing'
FAILED tests/tracing/test_pruning.py::TestTopoStatsPrune::test_prune_by_length[skeleton loop 1] - AssertionError: 
FAILED tests/tracing/test_dnatracing_single_grain.py::test_get_disordered_trace[linear topostats] - KeyError: 'dnatracing'
FAILED tests/tracing/test_pruning.py::TestTopoStatsPrune::test_prune_by_length[skeleton loop 2] - AssertionError: 
FAILED tests/tracing/test_dnatracing_single_grain.py::test_get_disordered_trace[circular lee] - KeyError: 'dnatracing'
FAILED tests/tracing/test_pruning.py::TestTopoStatsPrune::test_prune_by_length[skeleton linear 1] - AssertionError: 
FAILED tests/tracing/test_pruning.py::TestTopoStatsPrune::test_prune_by_length[skeleton linear 2] - AssertionError: 
FAILED tests/tracing/test_dnatracing_single_grain.py::test_get_disordered_trace[linear thin] - KeyError: 'dnatracing'
FAILED tests/tracing/test_pruning.py::TestTopoStatsPrune::test_prune_by_length[skeleton linear 3] - AssertionError: 
FAILED tests/tracing/test_dnatracing_single_grain.py::test_get_disordered_trace[circular topostats] - KeyError: 'dnatracing'
FAILED tests/tracing/test_dnatracing_single_grain.py::test_get_disordered_trace[circular thin] - KeyError: 'dnatracing'
FAILED tests/tracing/test_pruning.py::TestTopoStatsPruneMethods::test_prune_skeleton[Pruning by length and height disabled] - UnboundLocalError: cannot access local variable 'height_values' where it is not associated with a value
FAILED tests/tracing/test_pruning.py::TestTopoStatsPruneMethods::test_prune_skeleton[Height pruning based on mid(dle), height threshold 7.7e-19] - TypeError: unsupported operand type(s) for *: 'float' and 'NoneType'
FAILED tests/tracing/test_dnatracing_single_grain.py::test_get_disordered_trace[linear zhang] - KeyError: 'dnatracing'
FAILED tests/tracing/test_pruning.py::TestTopoStatsPruneMethods::test_prune_skeleton[Prune by default length pruning enabled (15% of overall length) removes branch] - UnboundLocalError: cannot access local variable 'height_values' where it is not associated with a value
FAILED tests/tracing/test_pruning.py::TestTopoStatsPruneMethods::test_prune_skeleton[Height pruning based on minimum, height threshold lower quartile - 1.5 x Interquartile range.] - TypeError: unsupported operand type(s) for *: 'float' and 'NoneType'
FAILED tests/tracing/test_pruning.py::TestTopoStatsPruneMethods::test_prune_skeleton[Length pruning enabled (25) removes everything!?!? Do we need a sanity check for this?] - UnboundLocalError: cannot access local variable 'height_values' where it is not associated with a value
FAILED tests/tracing/test_pruning.py::TestTopoStatsPruneMethods::test_prune_skeleton[Height pruning based on minimum, height threshold 5.0e-19] - TypeError: unsupported operand type(s) for *: 'float' and 'NoneType'
FAILED tests/tracing/test_dnatracing_single_grain.py::test_get_disordered_trace[circular zhang] - KeyError: 'dnatracing'
FAILED tests/tracing/test_pruning.py::TestTopoStatsPruneMethods::test_prune_skeleton[Height pruning based on median, height threshold 8.0e-19] - TypeError: unsupported operand type(s) for *: 'float' and 'NoneType'
==================================================== 45 failed, 451 passed, 23 skipped, 6 xfailed, 7 errors in 36.94s ====================================================

NB - Some tests have also been skipped or marked as expected to fail which will also need reinstating.

I'm also confused why this hasn't triggered the CI tests to be run. Some PRs subsequent to #925 which was meant to run CI tests on all pull requests have triggered them, but some haven't, I'll be fixing that.

SylviaWhittle commented 1 month ago

Windows just not liking installing tk, repeating until it passes.

SylviaWhittle commented 1 month ago

Okay I give up

if I use a different backend to appease windows, it fails on the other OSs

image
SylviaWhittle commented 1 month ago

Reverted commit.

The tests pass on everything except Windows python 3.9 so we just ditch that support please?

SylviaWhittle commented 1 month ago

@ns-rse Do you have any thoughts on this? Windows 3.9 fails due to tk issues, I tried using a different matplotlib backend but that caused Ubuntu to fail, and I'm scared of adding platform-specific code (backends for different platforms)

3.10 fails due to data type issues but I need to re-look at that as I've forgotten what I checked on Friday

ns-rse commented 1 month ago

The tests pass on everything except Windows python 3.9 so we just ditch that support please?

They also failed on Windows 3.10 with the following...

5830 FAILED tests/tracing/test_disordered_tracing.py::test_trace_image_disordered[catenane] - AssertionError: Attributes of DataFrame.iloc[:, 3] (column name="branch_type") are different
5831
5832 Attribute "dtype" are different
5833 [left]:  int32
5834 [right]: int64
5835 !!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!
5836 ====== 1 failed, 560 passed, 4 skipped, 2 warnings in 215.91s (0:03:35) =======
5837 Error: Process completed with exit code 1.

Could add in check_dtype = False to the pd.testing.assert_fames_equal() but this could be a slippery slope. Perhaps a better solution is to downcast the dtype of columns (I can't see we'll ever need int64 for the branch_type variable).

Thus after data frames are concatenated on line 344 of disordered_tracing.py we could...

disordered_tracing_stats = pd.concat((disordered_tracing_stats, skan_df))
disordered_tracing_stats = disordered_tracing_stats.astype({"branch_type": 'int32'})

However, this will also require that the same step is done when reading the CSV using pd.read_csv() and so the test itself will need to have a dictionary of the column names and dtype that they should be read as.

This creates a bit more work in my view and seems like the sort of situation that pytest-regtest was designed to handle. :balance_scale:

SylviaWhittle commented 1 month ago

The tests pass on everything except Windows python 3.9 so we just ditch that support please?

They also failed on Windows 3.10 with the following...

5830 FAILED tests/tracing/test_disordered_tracing.py::test_trace_image_disordered[catenane] - AssertionError: Attributes of DataFrame.iloc[:, 3] (column name="branch_type") are different
5831
5832 Attribute "dtype" are different
5833 [left]:  int32
5834 [right]: int64
5835 !!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!
5836 ====== 1 failed, 560 passed, 4 skipped, 2 warnings in 215.91s (0:03:35) =======
5837 Error: Process completed with exit code 1.

Could add in check_dtype = False to the pd.testing.assert_fames_equal() but this could be a slippery slope. Perhaps a better solution is to downcast the dtype of columns (I can't see we'll ever need int64 for the branch_type variable).

Thus after data frames are concatenated on line 344 of disordered_tracing.py we could...

disordered_tracing_stats = pd.concat((disordered_tracing_stats, skan_df))
disordered_tracing_stats = disordered_tracing_stats.astype({"branch_type": 'int32'})

However, this will also require that the same step is done when reading the CSV using pd.read_csv() and so the test itself will need to have a dictionary of the column names and dtype that they should be read as.

This creates a bit more work in my view and seems like the sort of situation that pytest-regtest was designed to handle. ⚖️

Yeah I will fix the type issue, by forcing the dtype as the data goes into the dictionary.

Do you have any thoughts / ideas about the matplotlib tk / tcl errors that seem to be failing the rest of the tests the majority of the time? I've tried a different backend but this breaks ubuntu :(

ns-rse commented 1 month ago

Do you have any thoughts / ideas about the matplotlib tk / tcl errors that seem to be failing the rest of the tests the majority of the time? I've tried a different backend but this breaks ubuntu :(

I think I've seen these quite a few times and from memory its when setting up/installing packages that things fail. I typically just have to manually re-run the tests.

Python 3.9 is the minimum version of Python and I think for the sake of not having to explain to people how to install and upgrade Python we should support the minimum version.

MaxGamill-Sheffield commented 1 month ago

I think I've seen these quite a few times and from memory its when setting up/installing packages that things fail. I typically just have to manually re-run the tests.

@ns-rse, @SylviaWhittle says this is on every re-run hence why she can't just push through. Is there a way to go into the test machines to manually ensure the package installs work? If not and as this is the highest priority, is dropping 3.9 support alright?

ns-rse commented 1 month ago

@MaxGamill-Sheffield The tests are failing occasionally because of tcl/tk issues (and this is something I've observed many times over the last year or two). Of the two that are currently showing as failed in this Pull Request both have failed on Windows Python 3.9 and 3.10 because of a dtype issue on reading the CSV file into a Pandas Dataframe and testing equality.

Dropping Python 3.9 support won't address this particular issue as it is a Windows issue.

A quick search led me to this thread which suggests that, unsurprisingly, the underlying problem encountered here is with Numpy (Pandas Dataframes are all essentially Numpy arrays).

ns-rse commented 1 month ago

@MaxGamill-Sheffield P.S. - The way to go into the GitHub runners is via the tmate action I created a Pull Request for and have just merged. But you can always look at the log file and see what has been installed under the Python setup section (needs expanding).

ns-rse commented 1 month ago

@MaxGamill-Sheffield Commit e59d1bd35059298c8f0794ab4f36d63fc3d9bc0c may have had some unintended consequences as other tests are now failing.

Somehow I see the same errors on the PR #949 even though e59d1bd35 was made after I branched as it doesn't show in the log for that branch as I branched from 0ad6ed3b8 * origin/maxgamill-sheffield/800-better-tracing Revert "Try to appease the worse operating system... (pyqt5 matplotlib backend)" where the test suite passed on both GNU/Linux and OSX.

The failures on Windows do seem to be due to dtype errors, at least those that I've had eyes on here on GitHub's Continuous Integration. If there are others that have cropped up I'd suggest making the changes on a branch to merge them into maxgamill-sheffield/800-better-tracing as this branch is itself currently closing in on being ready to merge to main and committing directly to it is probably a bad idea.

SylviaWhittle commented 1 month ago

Weirdly MacOS runner seems to time out on tmate initialisation... image

ns-rse commented 1 month ago

But tmate only runs if there has been an earlier failure which there has as we now see the tests fail with...

Error: test_interpolate_height_profile_images[Skeleton loop 1]

AssertionError: 
Arrays are not almost equal to 12 decimals

Mismatched elements: 104 / 104 (100%)
Max absolute difference: 10.53686996
Max relative difference: 9.22337208e+18
 x: array([ 4.307918269844,  4.8637[168](https://github.com/AFM-SPM/TopoStats/actions/runs/11334618627/job/31521176486?pr=932#step:5:169)38544,  5.285009481818,  5.534734846213,
        5.558195073897,  5.392339586946,  5.071938213424,  4.62108600[173](https://github.com/AFM-SPM/TopoStats/actions/runs/11334618627/job/31521176486?pr=932#step:5:174)9,
        4.09936482494 ,  3.548273814461,  2.995079890958,  2.504882276258,...
 y: array([4.67065435e-19, 5.27325236e-19, 5.73001876e-19, 6.00077154e-19,
       6.02620717e-19, 5.84638629e-19, 5.49900643e-19, 5.01019148e-19,
       4.44454025e-19, 3.84704618e-19, 3.24727212e-19, 2.71579881e-19,...
tests/measure/test_height_profiles.py::test_interpolate_height_profile_images[Skeleton loop 1] FAILED [ 17%]

...which looks like the order of magnitude change in e59d1bd.

I'll employ git bisect tomorrow to work out what has caused this change and then sort out my #949 in a similar manner.

SylviaWhittle commented 1 month ago

Why was the value of scale considered to be the cause of the Windows int32 v int64 error with dtype?

The affected variable was branch_type in the data frames that were being compared so nothing to do with height profiles.

These changes have affected the tests/measure/test_height_profiles.py::test_interpolate_height_profile_images because the same skeletons and height profiles are used in those tests.

It is worth running pytest before making pull requests and the introduction of pytest --testmon should have helped with that but may not be setup locally. Also branch and PR rather than committing directly to what is a development branch.

That commit was made to fix a pruning test that fails exclusively on windows that I flagged on friday, it appeared to be floating point arithmetic on values of order ~1e-19 (posting from windows so don't have the screenshot to hand) Max found that it was likely able to be fixed by just increasing the values used in the pruning heights to be less prone to being in the floating point error range ie, by setting them to be order ~1e-1

ns-rse commented 1 month ago

That commit was made to fix a pruning test that fails exclusively on windows that I flagged on friday, it appeared to be floating point arithmetic on values of order ~1e-19 (posting from windows so don't have the screenshot to hand) Max found that it was likely able to be fixed by just increasing the values used in the pruning heights to be less prone to being in the floating point error range ie, by setting them to be order ~1e-1

Ah, ok, not something I had eyes on as there was no issue documenting it which is why I was confused (nor a branch to make the fix and then merge into maxgamill-sheffield/800-better-tracing).

I've just run a git bisect (as its not something I have recourse to use often enough and I want more practice using it as its incredibly useful) and confirmed the tests are failing because of the changes introduced in [e59d1bd35]

e59d1bd35059298c8f0794ab4f36d63fc3d9bc0c is the first bad commit
commit e59d1bd35059298c8f0794ab4f36d63fc3d9bc0c (HEAD)
Author: Max Gamill <mcgamill1@sheffield.ac.uk>
Date:   2024-10-14 17:57:56 +0100
    Re-done pruning images so heights >> 1e-19 (now ~1) causing floating point errors on windows (we think)
 tests/conftest.py             |  2 +-
 tests/tracing/test_pruning.py | 21 ++++++++++-----------
 2 files changed, 11 insertions(+), 12 deletions(-)

Bisect Rest (1)
e59d1bd35 * @ Re-done pruning images so heights >> 1e-19 (now ~1) causing floating point errors on windows (we think)

Bisect Log (4)
git bisect start 'HEAD' '0ad6ed3b8'
c80459a47 bad Type hints, thanks @ns-rse
0ad6ed3b8 good Revert "Try to appease the worse operating system... (pyqt5 matplotlib backend)"

git bisect bad b228394e5582000c926c58beca41b936363fc04c
b228394e5 bad Type hints, thanks @ns-rse

git bisect bad e59d1bd35059298c8f0794ab4f36d63fc3d9bc0c
e59d1bd35 bad Re-done pruning images so heights >> 1e-19 (now ~1) causing floating point errors on windows (we think)

e59d1bd35059298c8f0794ab4f36d63fc3d9bc0c is the first bad commit

I made some comments on the PR which I see @MaxGamill-Sheffield has asnwered but the tests are still failing because the target arrays for the affected tests need updating too.

If these could be updated on a short lived branch with a pull request then these tests should in theory pass (although there may be an outstanding int32 v int64 issue which I'll address on #949).

MaxGamill-Sheffield commented 1 month ago

@ns-rse seems as though the tests are used for more than just in tracing, there as also failures in measure, and also now processing.

@SylviaWhittle will look at the processing errors, (which shouldn't have arisen from the pruning changes and that fixture shouldn't be used for the process scan tests).

I'll re-obtain the values for the now failing measure tests and push directly to here to fix them - this one is caused by the pruning fixture change :)

SylviaWhittle commented 1 month ago

The processing tests failing seem to be just due to some simple formatting stuff, I don't see any numerical changes..? @MaxGamill-Sheffield are you sure there was numerical changes? (I pulled latest commit and pip install -e .[dev,tests] before running)

test_process_scan::above: (below doesn't change)

image image image
MaxGamill-Sheffield commented 1 month ago

The processing tests failing seem to be just due to some simple formatting stuff, I don't see any numerical changes..? @MaxGamill-Sheffield are you sure there was numerical changes? (I pulled latest commit and pip install -e .[dev,tests] before running)

Oh weird - I say just push it then 🚀

SylviaWhittle commented 1 month ago

The tests have passed finally

SylviaWhittle commented 1 month ago

All checks passed, is this ready? 👀

SylviaWhittle commented 1 month ago

All checks passed 👀

MaxGamill-Sheffield commented 1 month ago

I think it's just the multi-version documentation (v2.0.0) docs that aren't building for this to go through.

Are they building for this version @ns-rse?

ns-rse commented 1 month ago

I think it's just the multi-version documentation (v2.0.0) docs that aren't building for this to go through.

Are they building for this version @ns-rse?

No, there was an error processing the v2.2.0 documentation that halted all subsequent tagged and main sets of documentation from building. The logs are here.

sphinx-multiversion is configured by the options at the bottom of docs/conf.py and uses patterns to determine which branches or versions to build documentation for. Typically we don't want most branches to have documentation built, we only want main and tagged releases.

# sphinx-multiversion https://holzhaus.github.io/sphinx-multiversion/master/configuration.html
smv_tag_whitelist = r"^v\d+.*$"  # Tags beginning with v#
smv_branch_whitelist = r"^main$"  # main branch
# If testing changes locally comment out the above and use the smv_branch_whitelist below instead. Replace the branch
# name you are working on ("ns-rse/466-doc-versions" in the example below) with the branch you are working on and run...
#
# cd docs
# sphinx-multiversion . _build/html
#
# smv_branch_whitelist = r"^(main|ns-rse/466-doc-versions)$"  # main branch
smv_released_pattern = r"^tags/.*$"  # Tags only
# smv_released_pattern = r"^(/.*)|(main).*$"  # Tags and HEAD of main
smv_outputdir_format = "{ref.name}"

This builds for the smv_tag_whitelist tags that match the regular expression (i.e. v<some number>) and for the smv_branch_whitelist the main branch. Thus maxgamill-sheffield/800-better-tracing won't match that and the documentation won't be built.

But I left notes on how to check the documentation for development branches and have worked out that the cause of the errors was the sphinx-autodoc-typehints package. Removing that and unpinning the restriction on sphinx-autoapi that was lingering and following the instructions to include the maxgamill-sheffield/800-better-tracing branch the documentation built locally for all specified branches and tags.

I'm now waiting to see if that works correctly on the workflow.

ns-rse commented 1 month ago

Documentation now builds for...

When building locally I've added in the maxgamill-sheffield/800-better-tracing branch and it includes the Advanced Documentation in the single Table of Contents...

Index

image

Which takes the reader to an index of advanced documentation (I figured this would over time grow)...

Advanced Documentation

image

Disordered Tracing

image

Nodestats

image

Ordered Tracing

image

Splining

image

MaxGamill-Sheffield commented 1 month ago

That's amazing thanks @ns-rse!

I've just removed the "DNA Tracing" subtitle from the page as we hope some of these can be used for samples other than DNA.

But after this is this all good to go then?

SylviaWhittle commented 1 month ago

Thank you so much @ns-rse ❤️

MaxGamill-Sheffield commented 1 month ago

Ahhhh the sphinx build fails on installing the fixed topoly version:


ERROR: Ignored the following versions that require a different python version: 1.10.0 Requires-Python <3.12,>=3.8; 1.10.0rc1 Requires-Python <3.12,>=3.8; 1.10.0rc2 Requires-Python <3.12,>=3.8; 1.10.1 Requires-Python <3.12,>=3.8; 1.21.2 Requires-Python >=3.7,<3.11; 1.21.3 Requires-Python >=3.7,<3.11; 1.21.4 Requires-Python >=3.7,<3.11; 1.21.5 Requires-Python >=3.7,<3.11; 1.21.6 Requires-Python >=3.7,<3.11; 1.6.2 Requires-Python >=3.7,<3.10; 1.6.3 Requires-Python >=3.7,<3.10; 1.7.0 Requires-Python >=3.7,<3.10; 1.7.1 Requires-Python >=3.7,<3.10; 1.7.2 Requires-Python >=3.7,<3.11; 1.7.3 Requires-Python >=3.7,<3.11; 1.8.0 Requires-Python >=3.8,<3.11; 1.8.0rc1 Requires-Python >=3.8,<3.11; 1.8.0rc2 Requires-Python >=3.8,<3.11; 1.8.0rc3 Requires-Python >=3.8,<3.11; 1.8.0rc4 Requires-Python >=3.8,<3.11; 1.8.1 Requires-Python >=3.8,<3.11; 1.9.0 Requires-Python >=3.8,<3.12; 1.9.0rc1 Requires-Python >=3.8,<3.12; 1.9.0rc2 Requires-Python >=3.8,<3.12; 1.9.0rc3 Requires-Python >=3.8,<3.12; 1.9.1 Requires-Python >=3.8,<3.12
ERROR: Could not find a version that satisfies the requirement topoly==1.0.2 (from topostats) (from versions: 1.0.3, 1.0.4)
ERROR: No matching distribution found for topoly==1.0.2
Error: Process completed with exit code 1.```
ns-rse commented 1 month ago

@MaxGamill-Sheffield I've just been going through the open issues, not all are incorporated in the #800 epic unfortunately.

I found #912 which I thought we could close but on closer reading the problem you highlighted was with the rendering of the docstring example which shows the dictionary structure. Have suggested how to resolve that.

There may be others, I've got about 20 minutes (whilst eating lunch) and then have to start work on my other project. It would be worth going through the last couple of pages of issues seeing what relates to this, whether it can be closed as its completed in some manner or not. I've been pretty poor at adding issues to the Milestone but did try to add them to #800 so they could be ticked off.

NB - I see the gh-pages CI is failing I'll fix that and next week I will be writing to the authors asking for updated versions of Topoly.

SylviaWhittle commented 1 month ago

Ahhhh the sphinx build fails on installing the fixed topoly version:

ERROR: Ignored the following versions that require a different python version: 1.10.0 Requires-Python <3.12,>=3.8; 1.10.0rc1 Requires-Python <3.12,>=3.8; 1.10.0rc2 Requires-Python <3.12,>=3.8; 1.10.1 Requires-Python <3.12,>=3.8; 1.21.2 Requires-Python >=3.7,<3.11; 1.21.3 Requires-Python >=3.7,<3.11; 1.21.4 Requires-Python >=3.7,<3.11; 1.21.5 Requires-Python >=3.7,<3.11; 1.21.6 Requires-Python >=3.7,<3.11; 1.6.2 Requires-Python >=3.7,<3.10; 1.6.3 Requires-Python >=3.7,<3.10; 1.7.0 Requires-Python >=3.7,<3.10; 1.7.1 Requires-Python >=3.7,<3.10; 1.7.2 Requires-Python >=3.7,<3.11; 1.7.3 Requires-Python >=3.7,<3.11; 1.8.0 Requires-Python >=3.8,<3.11; 1.8.0rc1 Requires-Python >=3.8,<3.11; 1.8.0rc2 Requires-Python >=3.8,<3.11; 1.8.0rc3 Requires-Python >=3.8,<3.11; 1.8.0rc4 Requires-Python >=3.8,<3.11; 1.8.1 Requires-Python >=3.8,<3.11; 1.9.0 Requires-Python >=3.8,<3.12; 1.9.0rc1 Requires-Python >=3.8,<3.12; 1.9.0rc2 Requires-Python >=3.8,<3.12; 1.9.0rc3 Requires-Python >=3.8,<3.12; 1.9.1 Requires-Python >=3.8,<3.12
ERROR: Could not find a version that satisfies the requirement topoly==1.0.2 (from topostats) (from versions: 1.0.3, 1.0.4)
ERROR: No matching distribution found for topoly==1.0.2
Error: Process completed with exit code 1.```

But if topoly > 1.0.2 then topology tests fail since topology >= 1.0.3 uses new nomenclature right?

Dunno what to do here

ns-rse commented 1 month ago

Dunno what to do here

I bumped the Python version to 3.12 from 3.9 to ensure latest versions of all the optional docs dependencies were being pulled in whilst trying to trouble shoot. Dropping to 3.11 should sort this.

ns-rse commented 1 month ago

The documentation lives! :zombie:

Still recommend reviewing Issues to see if there is anything that can be closed or hasn't been addressed (e.g. #912) but :crossed_fingers: