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

Porting hazen to python 3.9 #143

Merged
merged 72 commits into from
Dec 7, 2021
Merged

Porting hazen to python 3.9 #143

merged 72 commits into from
Dec 7, 2021

Conversation

tomaroberts
Copy link
Collaborator

See #142.

Currently, for python3.9, hazen requires conda package management as not all wheels working in pip. hazen can be run in 3.9, but some tools failing because of changes to old libraries, and possibly other issues.

TODO:

@tomaroberts tomaroberts self-assigned this Nov 15, 2021
@tomaroberts tomaroberts linked an issue Nov 15, 2021 that may be closed by this pull request
@tomaroberts tomaroberts marked this pull request as draft November 15, 2021 17:05
@github-actions
Copy link

github-actions bot commented Nov 15, 2021

@tomaroberts
Copy link
Collaborator Author

Reminder: OpenCV findContours() changed the number of output arguments between versions... Unhelpful -_-

https://stackoverflow.com/a/58244568

@tomaroberts
Copy link
Collaborator Author

tomaroberts commented Nov 16, 2021

tests/resolution/RESOLUTION passes
tests/resolution/eastkent fails
tests/resolution/philips fails

eastkent and philips failing here:

image

These 5 all have rescale_to_byte(), thresh() and find_square() in common. I'm pretty sure find_square is working, otherwise the RESOLUTION test set wouldn't pass.

In the eastkent/philips test_data, I think something might be happening with rescale_to_byte() or thresh() which is causing find_square() to output coordinates which don't match the test values. e.g.:

image

I've also noticed that there is a subtle different in the images output between hazen3.6 and hazen3.9:

Left = old hazen (3.6)
Right = new hazen (3.9)

image

The 3.9 images are smoother when loaded in – are they supposed to look like left or right?! Is the 'shimmer' in the 3.6 images an MR artefact or python artefact?

@tomaroberts
Copy link
Collaborator Author

Should add, current spatial_resolution results:

RESOLUTION
perfect match between py3.6/py3.9

eastkent
py3.6: FE = 0.98 / PE = 0.99
py3.9: FE = 0.99 / PE = 0.99

philips:
py3.6: FE = 0.49 / PE = 0.54
py3.9: FE = 0.49 / PE = 0.49

So pretty similar, seemingly indicating numerical/subtle threshold or contrast issue.

@@ -0,0 +1,47 @@
name: hazen-3.9-miniforge
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this file do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, shouldn't have spread between two places. Bit more context in #142.

Basically, installing some of our libraries (mainly scikit-image) doesnt work on M1 silicon via pip. So instead I made the venv in miniforge (just a smaller version of conda). The environment.yml is the equivalent to our normal requirements.txt.

I specifically made most of it install via pip anyway, so it was easy to see which ones work via pip and which only work via conda.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being a bit slow here. But doesn't the dockerisation of Hazen make this issue a moot point?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I should have made this comment on the issue when you first raised it.

hazenlib/spatial_resolution.py Show resolved Hide resolved
@hshuaib90
Copy link
Collaborator

tests/resolution/RESOLUTION passes tests/resolution/eastkent fails tests/resolution/philips fails

eastkent and philips failing here:

image

These 5 all have rescale_to_byte(), thresh() and find_square() in common. I'm pretty sure find_square is working, otherwise the RESOLUTION test set wouldn't pass.

I just checked and all the resolution tests are failing with your recent commit #866dc22 . find_square is the only code you've changed from develop branch according to the GitHub PR.

The 3.9 images are smoother when loaded in – are they supposed to look like left or right?! Is the 'shimmer' in the 3.6 images an MR artefact or python artefact?

I'm not sure why the appearance of the images changes between Python versions, but it is worrying. Need to see if anyone else can reproduce locally.

@tomaroberts
Copy link
Collaborator Author

I just checked and all the resolution tests are failing with your recent commit #866dc22 . find_square is the only code you've changed from develop branch according to the GitHub PR.

image

These are the latest results I'm getting from running pytest tests/

@tomaroberts
Copy link
Collaborator Author

I've just noticed the Git Actions pytest section where it's failing all the spatial_resolution tests, as you say.

I think the output is different to the tests I'm running locally on my laptop because the Git Actions test is building the code from requirements.txt with py3.6 and using the old OpenCV version, meaning that my alteration to findContours() is causing it to fail.

@tomaroberts
Copy link
Collaborator Author

tomaroberts commented Nov 18, 2021

@hshuaib90

I'm not sure why the appearance of the images changes between Python versions, but it is worrying. Need to see if anyone else can reproduce locally.

Have discovered that the matplotlib version makes a significant different on how the images are displayed...!

See images below, generated using identical python3.9 venvs, only difference is matplotlib versions:

Top: matplotlib==3.1.3 (which is used in main hazen version)
Bottom: matplotlib==3.4.3

Screenshot 2021-11-18 at 14 12 49

Matplotlib shouldn't be affecting the values in the underlying dicom pixel_arrays however as it's just for plotting, so the discrepancies in hazen output values is something else.

At the moment, I'm pretty sure that the hazen code is behaving consistently between the 3.6 build and my 3.9-miniforge build, but a subtle change in one of the libraries is giving us slightly different results, hence the tests are failing.

My feeling is that we just need to update the tests to reflect these slight changes in values.

environment-mplib3.1.3.yml.txt

@tomaroberts
Copy link
Collaborator Author

Updated requirements.txt to specify mistune version as m2r install was causing error. See: miyakogi/m2r#66

.github/workflows/cli-test.yml Outdated Show resolved Hide resolved
.github/workflows/tests_development.yml Show resolved Hide resolved
.github/workflows/tests_development.yml Outdated Show resolved Hide resolved
.github/workflows/tests_release.yml Outdated Show resolved Hide resolved
@tomaroberts tomaroberts added this to the 0.5.0 milestone Dec 7, 2021
@tomaroberts tomaroberts changed the base branch from develop to release/0.5.0 December 7, 2021 17:48
@tomaroberts tomaroberts merged commit 87837d4 into release/0.5.0 Dec 7, 2021
@tomaroberts
Copy link
Collaborator Author

December release of scikit-image added arm64 support so hazen install via pip now works on M1 Macs \o/

Thanks to @laurencejackson for additional assistance in OpenSSL installation, required for psycopg2-binary package and killing conda.

@tomaroberts tomaroberts linked an issue Dec 10, 2021 that may be closed by this pull request
@hshuaib90 hshuaib90 deleted the hazen-3.9-miniforge branch July 30, 2022 17:15
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.

Use Ibn Al-Haytham's image instead of CSC logo Update hazen to work with Python 3.9
3 participants