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

don't use texture_format auto if float textures not available on machine #3990

Merged
merged 2 commits into from Jan 20, 2022

Conversation

tlambert03
Copy link
Member

@tlambert03 tlambert03 commented Jan 19, 2022

Description

hopefully fixes #3988

@alonyan, would be great if you could test this out. this should work to install:

pip install -U git+https://github.com/tlambert03/napari.git@fallback-float-texture

Type of change

  • Bug-fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

References

How has this been tested?

  • example: the test suite for my feature covers cases x, y, and z
  • example: all tests pass with my change

Final checklist:

  • My PR is the minimum possible work for the desired functionality
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • If I included new strings, I have used trans. to make them localizable.
    For more information see our translations guide.

@tlambert03 tlambert03 changed the title don't use texture_format auto if float not available don't use texture_format auto if float textures not available on machine Jan 19, 2022
@codecov
Copy link

codecov bot commented Jan 19, 2022

Codecov Report

Merging #3990 (411703e) into main (1363bd4) will increase coverage by 0.01%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3990      +/-   ##
==========================================
+ Coverage   83.17%   83.18%   +0.01%     
==========================================
  Files         587      587              
  Lines       48368    48388      +20     
==========================================
+ Hits        40228    40252      +24     
+ Misses       8140     8136       -4     
Impacted Files Coverage Δ
napari/_vispy/layers/image.py 97.03% <66.66%> (-0.71%) ⬇️
napari/_vispy/utils/gl.py 88.09% <100.00%> (+1.25%) ⬆️
napari/utils/context/_layerlist_context.py 100.00% <0.00%> (ø)
napari/layers/_tests/test_layer_actions.py 95.23% <0.00%> (+0.59%) ⬆️
napari/layers/_layer_actions.py 68.29% <0.00%> (+8.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1363bd4...411703e. Read the comment docs.

@alonyan
Copy link

alonyan commented Jan 20, 2022

That works. Thanks!

@tlambert03
Copy link
Member Author

nice!

napari/utils/info.py Outdated Show resolved Hide resolved
Copy link
Contributor

@brisvag brisvag left a comment

Choose a reason for hiding this comment

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

I agree that the check lives better in utils/gl.py. Other than that, LGTM!

Copy link
Contributor

@alisterburt alisterburt left a comment

Choose a reason for hiding this comment

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

nice, super clean!

@alisterburt alisterburt merged commit 0f5281f into napari:main Jan 20, 2022
@tlambert03 tlambert03 deleted the fallback-float-texture branch January 20, 2022 21:46
@nclack nclack mentioned this pull request Jan 25, 2022
11 tasks
jni added a commit that referenced this pull request Feb 13, 2024
…#6652)

# Description

In #6411, we missed these lines:

https://github.com/napari/napari/blob/4f4c063ae5dd79d6d188e201d44b8d57eba71909/napari/_vispy/layers/image.py#L25-L32

These were added in #3990 because 'auto' has a slightly different
meaning in VisPy than *really* fully auto: it chooses the default
texture dtype for the input NumPy array's dtype. For float arrays, this
is float. However, if your OpenGL implementation doesn't support float
textures, VisPy will raise.

Instead, passing `texture_format=None` when creating the ImageNode tells
VisPy to transform the data to whatever texture format it sees fit.

In #6411, when getting the VisPy node for a given dtype, we only check
for `texture_format != 'auto'` as the "catch-all" texture format. But,
in fact, if we are on a machine that doesn't support float32 textures,
by this point in the code the format has been changed to None,
incorrectly triggering these lines:

https://github.com/napari/napari/blob/89f8194d3fa4eef620755804806ac69ef684df63/napari/_vispy/layers/image.py#L59-L73

and causing a ValueError.

This PR fixes that by also checking for None in that same clause.

This PR also adds a test by monkeypatching the function that checks for
float32 texture support.

# References

#3988
#3990

---------

Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Czaki added a commit that referenced this pull request Feb 20, 2024
…#6652)

# Description

In #6411, we missed these lines:

https://github.com/napari/napari/blob/4f4c063ae5dd79d6d188e201d44b8d57eba71909/napari/_vispy/layers/image.py#L25-L32

These were added in #3990 because 'auto' has a slightly different
meaning in VisPy than *really* fully auto: it chooses the default
texture dtype for the input NumPy array's dtype. For float arrays, this
is float. However, if your OpenGL implementation doesn't support float
textures, VisPy will raise.

Instead, passing `texture_format=None` when creating the ImageNode tells
VisPy to transform the data to whatever texture format it sees fit.

In #6411, when getting the VisPy node for a given dtype, we only check
for `texture_format != 'auto'` as the "catch-all" texture format. But,
in fact, if we are on a machine that doesn't support float32 textures,
by this point in the code the format has been changed to None,
incorrectly triggering these lines:

https://github.com/napari/napari/blob/89f8194d3fa4eef620755804806ac69ef684df63/napari/_vispy/layers/image.py#L59-L73

and causing a ValueError.

This PR fixes that by also checking for None in that same clause.

This PR also adds a test by monkeypatching the function that checks for
float32 texture support.

# References

#3988
#3990

---------

Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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.

Float dtypes no longer work on some hardware (without float textures) as of 0.4.13
5 participants