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

Remove old async octree #28

Closed
wants to merge 34 commits into from
Closed

Remove old async octree #28

wants to merge 34 commits into from

Conversation

kcpevey
Copy link
Owner

@kcpevey kcpevey commented Mar 23, 2023

Description

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
  • example: I check if my changes works with both PySide and PyQt backends
    as there are small differences between the two Qt bindings.

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.

@kcpevey
Copy link
Owner Author

kcpevey commented Mar 23, 2023

I think I should also remove this since its already covered by every other test (switching async on and off is now much easier). If I do that, I'll also remove this since its slower than just running with a marker and the cli option was really just for tox.

I may also rename that mark to async since the purpose is no longer "you can only run these under async mode" but now it is "I just want to run the async tests".

There are also a series of tests that were marked sync_only but now that there isn't a global async, it didn't seem reasonable to keep it. https://github.com/kcpevey/napari/pull/28/files#diff-e8b12d6af4f66a0a4c2a4841e10580ea5aad2c8fd6b7443502b70be88e883cfcR13

@kcpevey
Copy link
Owner Author

kcpevey commented Mar 23, 2023

@andy-sweet one more question - I could write a make_napari_viewer with the async setting turned on and then run the whole test suite (or just the image tests?) with both async and sync. Thoughts?

napari/_qt/qt_viewer.py Outdated Show resolved Hide resolved
@andy-sweet
Copy link
Collaborator

Good to see that this minor surgery is pretty straightforward and tests continue to pass.

The only thing I want to see resolved before we open this on napari/napari is a summary of what the existing test configs (i.e. sync-only async-only) mean before and after this PR. I'm pretty confused about what actually happens and ideally need a fairly detailed description with links to code - if that exists elsewhere, feel free to redirect me to it.

My intuition is that we don't need any test config flags after removing the old async/octree. The new async doesn't have any special import behavior and can be enabled/disabled dynamically at runtime. It's an experimental feature, so I'd expect most tests to run without it, but would expect some tests to run with it. Those tests that need it don't need a special pytest config, they can just use a shared setup function.

I would not expect to run all tests with async enabled, since anything that actually does slicing could fail unpredictably.

@andy-sweet
Copy link
Collaborator

andy-sweet commented Mar 27, 2023

Also when this PR is opened on napari/napari, I think you should initially not remove all the experiment/octree source code itself. Instead, I'd open it without those removals so we bring attention to the parts of the integration that are changing/removing. We could then suggest removing that code since it is private and not reachable.

@kcpevey
Copy link
Owner Author

kcpevey commented Mar 28, 2023

The only thing I want to see resolved before we open this on napari/napari is a summary of what the existing test configs (i.e. sync-only async-only) mean before and after this PR. I'm pretty confused about what actually happens and ideally need a fairly detailed description with links to code - if that exists elsewhere, feel free to redirect me to it.

My intuition is that we don't need any test config flags after removing the old async/octree. The new async doesn't have any special import behavior and can be enabled/disabled dynamically at runtime. It's an experimental feature, so I'd expect most tests to run without it, but would expect some tests to run with it. Those tests that need it don't need a special pytest config, they can just use a shared setup function.

I agree with you - we don't need special config. I did the special async flag because it allows pytest to use it the marker as a cli command which is how tox is running it. I will see if I can get tox to run with a marker and then remove that cli flag.

I tried to add some explanations of this, but I obviously need to add more either in the comments or the PR itself. It took significant time to understand the old setup so documenting these changes is definitely important. The good news is that is MUCH simpler now.

@kcpevey
Copy link
Owner Author

kcpevey commented Mar 28, 2023

Also when this PR is opened on napari/napari, I think you should initially not remove all the experiment/octree source code itself. Instead, I'd open it without those removals so we bring attention to the parts of the integration that are changing/removing. We could then suggest removing that code since it is private and not reachable.

I'm not sure I understand. You want to remove the usage of octree, but not the files themselves? I guess that might be possible if I modified all the import statements. I dont see the benefit though. Do you just feel like the PR touches too many files?

@andy-sweet
Copy link
Collaborator

I'm not sure I understand. You want to remove the usage of octree, but not the files themselves? I guess that might be possible if I modified all the import statements. I dont see the benefit though. Do you just feel like the PR touches too many files?

Yeah, my worry is that the topic would become more about the removal of the octree code (some of which might be used in the future), rather than removal of the setting and integration into napari. That code is obviously still in the repo after we remove it, so it doesn't really matter. I guess removing the settings will cause a problem with some of the tests that you're removing though?

@kcpevey
Copy link
Owner Author

kcpevey commented Mar 28, 2023

removing the settings will cause a problem with some of the tests that you're removing though?

Yes, although you might word it differently. The settings have been removed (and the code no longer functional) so the tests needed to be removed as well.

Ok so what I will do is

  • add back in the deleted files
  • keep all of the other code changes where I import and use any async or octree stuff (i.e. the functionality is removed, the root codebase remains, but the implementation is removed)
  • leave the tests as deleted since the code will no longer run (alternative to this is that I add a skip marker to all these)

I should also note that there remains some other mentions of chunk here and there in the codebase. I wasn't sure if I should continue with the full removal since this originally an experiment. And it seemed I was in a good stopping point since tests were passing.

If this seems like a reasonable solution, should continue with full removal?

@andy-sweet
Copy link
Collaborator

I think removing the tests is fine. But let's keep the implementation just in case others are depending on it for in development stuff (e.g. @kephale). Actually, Kyle would be a great person to get some eyes on this either before or after we move it to napari/napari.

No need to handle all references to the old async/octree stuff here. There will definitely be a follow-up PR where we work out how to flatten/simplify the variety of classes associated with those.

@kephale
Copy link

kephale commented Mar 29, 2023

Initial reaction to the PR:
image

My main expected use for this code is to use it as a reading reference. The OO-style is heavy, and the code we're developing as a replacement (over here https://github.com/kephale/napari/tree/poor-mans-octree) is in a more functional-style with some other key differences.

Initially I was expecting to try to reuse the TextureAtlas. However, our alternative 2D tiled loading behaves pretty reasonably without the TextureAtlas. Also, due to the implementation differences it will be easier to just write our own when we need to do texture lookups on the GPU.

To be clear, I do not expect to need this code and am fine with removal.

napari/conftest.py Outdated Show resolved Hide resolved
andy-sweet and others added 19 commits April 6, 2023 12:19
This skips version 6.4.3 of pyside6 when testing to avoid the problems
described in napari#5657. It does not close that issue, but at least reduces
noise on the PR tests.

- [x] Bug-fix (non-breaking change which fixes an issue)

Related to napari#5657

- [x] all tests pass with my change
Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

# Description

This PR stores the layer visibility states prior to toggling layers and
then checks against it to ensure that layers are toggled relative to
that initial state. This makes sure they are not double toggled if
linked layers are involved.
Also added a test for the interaction between linked layers and
toggle_visibility that fails without this PR but now passes.

## Type of change
<!-- Please delete options that are not relevant. -->
- [x] Bug-fix (non-breaking change which fixes an issue)

# References
Closes napari#5655 

# How has this been tested?
<!-- Please describe the tests that you ran to verify your changes. -->
- [x] I added a test that fails on main but passes here
- [x] all tests pass with my change 

## Final checklist:
- [x] My PR is the minimum possible work for the desired functionality

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>
…ues (napari#5635)

# Description
This fixes an off-by-one error on Surface layers extent data, which was
causing an IndexError slicing that dimension.

## Type of change
- [x] Bug-fix (non-breaking change which fixes an issue)

# References
closes napari#5620
Disussed in [community meeting on
2023-03-15](https://hackmd.io/BXWDZ3i8Q6OAEASrkaSNIQ?view#2023-03-15)

# How has this been tested?
- [x] all tests pass with my change (though I had to change two of them)
- [x] thanks to @brisvag who [tested to make sure the mesh was in the
correct spot wrt an
image](napari#5620 (comment))

## Final checklist:
- [x] My PR is the minimum possible work for the desired functionality
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have made corresponding changes to the documentation
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] If I included new strings, I have used `trans.` to make them
localizable.
For more information see our [translations
guide](https://napari.org/developers/translations.html).

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
<!-- What does this pull request (PR) do? Why is it necessary? -->
<!-- Tell us about your new feature, improvement, or fix! -->
<!-- If your change includes user interface changes, please add an
image, or an animation "An image is worth a thousand words!" -->
<!-- You can use https://www.cockos.com/licecap/ or similar to create
animations -->

* `INP` flake8-no-pep420 - The pep 420 is something that part of us may
remember as the reason why napari stop working at some moment because of
some problematic files in `site-packages`.

 * `PYI` flake8-pyi - better type annotation
* `Q`, flake8-quotes - enforce quoting, but the rule Q000 is disabled
(see napari#5589).
* `RSE` # flake8-raise - simplify raise code by remove `()` when
obsolete.
* `RET` # flake8-return - simplify code readability by forcing proper
return schema.
 * `TID` # flake8-tidy-imports - replacement for absolutify imports
* `TRY` # tryceratops - enforce rules on exception raises like keeping
in try only this that could raise exception (see why it could create
problems in napari#5577).
* `ICN` # flake8-import-conventions - enforce import convention for
alias (like `import numpy as np`)
 * `RUF`# ruff specific rules -

<!-- Please delete options that are not relevant. -->
- [x] Maintenance

<!-- What resources, documentation, and guides were used in the creation
of this PR? -->
<!-- If this is a bug-fix or otherwise resolves an issue, reference it
here with "closes #(issue)" -->

<!-- Please describe the tests that you ran to verify your changes. -->
- [ ] example: the test suite for my feature covers cases x, y, and z
- [ ] example: all tests pass with my change
- [ ] example: I check if my changes works with both PySide and PyQt
backends
      as there are small differences between the two Qt bindings.

- [ ] 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](https://napari.org/developers/translations.html).
The script was not handling string with more than one letter prefix, in
more details, it was trimming only the first letter of the prefix, and
thus when evaluating for example a rf"" f-string, the f would not be
removed, and the eval would look try to interpolate the ftrings in the
context of the function that then would fail.

Now we keep the prefix, but remove "f" from it.

As a second side effect, this also mean that we now distinguish
bytestrings (`b""`), anc can just skip over those as they should never
be translated.

Another addition in the interactive translation updating script, is that
it now print at the beginning an estimate of the number of places that
are missing translation or need to be added to the ignore file.

The translation tests will still fail, but this is because there are
missing translation/entries in the ignore file.

I did not update he ignore file yet to keep things reviewable.

# Description
<!-- What does this pull request (PR) do? Why is it necessary? -->
<!-- Tell us about your new feature, improvement, or fix! -->
<!-- If your change includes user interface changes, please add an
image, or an animation "An image is worth a thousand words!" -->
<!-- You can use https://www.cockos.com/licecap/ or similar to create
animations -->

## Type of change
<!-- Please delete options that are not relevant. -->
- [ ] 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
<!-- What resources, documentation, and guides were used in the creation
of this PR? -->
<!-- If this is a bug-fix or otherwise resolves an issue, reference it
here with "closes #(issue)" -->

# How has this been tested?
<!-- Please describe the tests that you ran to verify your changes. -->
- [ ] example: the test suite for my feature covers cases x, y, and z
- [ ] example: all tests pass with my change
- [ ] example: I check if my changes works with both PySide and PyQt
backends
      as there are small differences between the two Qt bindings.  

## 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](https://napari.org/developers/translations.html).
In many cases we actually want to only catch ModuleNotFoundError to
indicate that the module is not installed, catching/suppressing
ImportError also tend to hide deeper import issues and may make things
harder to debug.
…apari#5669)

# Description

This fixes issues with Pan/Zoom buttons (layer controls) not working and
the spacebar keybinding to activate pan/zoom mode also not working.
In napari#4894 there was some refactoring
which introduced the bugs, see my investigation here:
napari#5654

This PR fixes the typo, lack of proper layer types, and now moves the
`hold_for_pan_zoom` to be a Viewer keybinding. As a result, this
keybinding isn't hard-coded anymore, but is now settable in the
Preferences > Shortcuts.

## Type of change
<!-- Please delete options that are not relevant. -->
- [x] Bug-fix (non-breaking change which fixes an issue)
- [x] 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
closes napari#5654

# How has this been tested?
Good question! All tests were passing before this PR, despite the fact
that things were broken. So I'm open to suggestions how to improve tests
so we catch stuff better.


## Final checklist:
- [x] 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](https://napari.org/developers/translations.html).
# Description
closes napari#5677

I tracked down the source of the added Transform and Pan/zoom in the
Viewer keybindings.
Those are actions/keybinds added in
napari#4894 for the Tracks layer. But
Tracks layer wasn't added as an option in the shortcuts table, so they
appear in the default (Viewer).
With this PR, I import Tracks and add it to the dropdown so Tracks has
it's own table and now the actions appear in the proper table, rather
than under Viewer keybindings.

<img width="1198" alt="image"
src="https://user-images.githubusercontent.com/76622105/228287187-189cb745-fb6e-4c31-93f1-6ba22e965a56.png">

<img width="1198" alt="image"
src="https://user-images.githubusercontent.com/76622105/228287227-da2642b7-8f76-4451-bdfa-e6801f47400a.png">



## Type of change
<!-- Please delete options that are not relevant. -->
- [x] 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?
<!-- Please describe the tests that you ran to verify your changes. -->
- [ ] example: the test suite for my feature covers cases x, y, and z
- [x] example: all tests pass with my change
- [ ] example: I check if my changes works with both PySide and PyQt
backends
      as there are small differences between the two Qt bindings.  

## Final checklist:
- [x] 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](https://napari.org/developers/translations.html).
…apari#5682)

# Description
Alternative to napari#5662. This is actually addressing the issue, instead of
inventing new problems to solve :P

This fixes the original problem; this will now work as intended and
correctly update the dropdown menu and the global `AVAILABLE_COLORMAPS`
dict.


```
import napari
import numpy as np
v = napari.Viewer()
i = v.add_image(np.random.rand(10, 10, 10), colormap=np.random.rand(5, 3))
s = v.add_surface((np.random.rand(5, 3) * 50, np.random.choice(range(5), (3, 3))), colormap=np.random.rand(5, 3))
```


A side effect of the current version is that instead of `custom`, new
unnamed colormaps are named `[unnamed colormap i]` where `i` is an
incremental index. We can change the default to `custom`, if preferred,
but for now I opted for minimal code changes for easier review.

Note that a notable change (read: bug fix) of this PR is that the code
above generate 2 different colormaps, instead of overwriting a single
`custom` colormap.

cc @andy-sweet 
<!-- What does this pull request (PR) do? Why is it necessary? -->
<!-- Tell us about your new feature, improvement, or fix! -->
<!-- If your change includes user interface changes, please add an
image, or an animation "An image is worth a thousand words!" -->
<!-- You can use https://www.cockos.com/licecap/ or similar to create
animations -->

## Type of change
<!-- Please delete options that are not relevant. -->
- [x] 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
<!-- What resources, documentation, and guides were used in the creation
of this PR? -->
<!-- If this is a bug-fix or otherwise resolves an issue, reference it
here with "closes #(issue)" -->

# How has this been tested?
<!-- Please describe the tests that you ran to verify your changes. -->
- [ ] example: the test suite for my feature covers cases x, y, and z
- [ ] example: all tests pass with my change
- [ ] example: I check if my changes works with both PySide and PyQt
backends
      as there are small differences between the two Qt bindings.  

## 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](https://napari.org/developers/translations.html).
# Description

Currently, when PRs have long and detailed descriptions (a good
thing!!!), it's
hard to find the reference to the issue(s) that they close. This updates
the
template so that those issues are linked right at the top, before the
description.

I think a lot of the stuff at the bottom of the PR template is cruft (it
was
created a long time ago), but we can discuss that in separate
issues/PRs.
Now that `lazy_loader` is being released as a package, it may be easier
to depend on it than to vendor the code.

@tlambert03 has implemented a feature in `lazy_loader` that allows lazy
loading, while also letting type checkers do their jobs. See
https://scientific-python.org/specs/spec-0001/#type-checkers

I could transition the napari lazy imports to the stub loader in a next
PR, if that's desired.
…ng) (napari#5672)

# Description

This adds an action analogous to the current `new_label` action (current
keybind `M`) to set the current label to background (0). The default
keybind has been set to `B` because it's close to space and close M,
plus B for `background`. Open to other suggestions.
This is sort of similar to `activate_labels_erase_mode` but in a lot of
ways more powerful, because you can use the `fill` tool to erase
contiguous areas.

## Type of change
<!-- Please delete options that are not relevant. -->
- [ ] Bug-fix (non-breaking change which fixes an issue)
- [x] 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
Closes napari#5666
First came up in a docs PR:
napari/docs#125 (comment)

# How has this been tested?
<!-- Please describe the tests that you ran to verify your changes. -->
- [x] example: I added a test
- [x] example: all tests pass with my change
- [ ] example: I check if my changes works with both PySide and PyQt
backends
      as there are small differences between the two Qt bindings.  

## Final checklist:
- [x] 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: will
update napari/docs#125 once the default keybind
is settled.
- [ ] 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](https://napari.org/developers/translations.html).
* Add back some things [skip ci]

* Remove pytest markers for old async

* Deprecate loaded

* Empty commit to rerun CI

* Empty commit to rerun CI

* Ignore deprecated introspection
pyproject.toml Outdated
"ignore::DeprecationWarning:shibokensupport",
"ignore::DeprecationWarning:ipykernel",
"ignore::DeprecationWarning:tensorstore",
"ignore:Layer.loaded is deprecated:DeprecationWarning:",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment above suggests that napari deprecation warnings should not go here. I added individual filters to tests related to this deprecation. But we should probably use pytest.warns as the comment suggests so it can be removed when not needed.

Another solution (which I probably prefer for this PR) is to not deprecate loaded at all - it's not a publicly settable property and we can always return True correctly. What do you think?

Copy link
Owner Author

@kcpevey kcpevey Apr 11, 2023

Choose a reason for hiding this comment

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

The comment above suggests that napari deprecation warnings should not go here. I added individual filters to tests related to this deprecation. But we should probably use pytest.warns as the comment suggests so it can be removed when not needed.

Fair. Admittedly I wasn't paying close enough attention to all that. You missed at least one test that needed it (in qt_viewer somewhere). Going the pytest.warns route will be good here since it will cause us to fix it when its no longer needed (pending discussion below)

Another solution (which I probably prefer for this PR) is to not deprecate loaded at all - it's not a publicly settable property and we can always return True correctly. What do you think?

Do you mean leave the loaded property in altogether - both in base and in image.py? We deprecated chunk_receiver too. I think whatever we do for one should be done for the other. Also, the files for octree still exist in this PR.

It kind of feels like this is the right time to deprecate. Then a followup PR can remove everything, including the deprecations. The alternative would be to not deprecate now, then deprecate and remove all the files in a separate PR, then in a third PR, remove the deprecations. Does that make sense?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll leave the decision to you here.

GUI thread.

Set NAPARI_ASYNC=1 to turn on async loading with default settings.
Deprecated.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this was my change, but we might want to add a bit more detail here. E.g. that they should use the setting directly instead.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Actually... I reused the same environment variable name to provide consistency with what people expect. I am not 100% clear on how all the settings stuff works, but its my understanding that this allows it to be set via env var. If that's true then the statement Set NAPARI_ASYNC=1 to turn on async loading is also true.

Copy link
Collaborator

@andy-sweet andy-sweet Apr 11, 2023

Choose a reason for hiding this comment

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

Understood. My general point here is that if we're deprecating this and returning some safe, fixed value, then we should explain why it was deprecated (because the corresponding functionality was removed) and what they should use instead. I'll leave the details to you.

The other high level option is just to remove these in which case we don't have to worry about these things. You could float that idea with the community - even though this is part of the public API, maybe people will be OK with it?

2) Set NAPARI_OCTREE=/tmp/config.json use a config file.

See napari/utils/_octree.py for the config file format.
Deprecated.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

Agreed.

self.chunk_receiver = QtChunkReceiver(self.layers)
else:
self.chunk_receiver = None
self.chunk_receiver = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

As it stands, this overwrites the defined function that returns None with None itself.

@andy-sweet
Copy link
Collaborator

andy-sweet commented Apr 11, 2023

Not sure I can add more comments to this PR without just spinning us both in circles more. So it's time to open it up on napari/napari and get wider feedback. I still suggest removing / cleaning up some of the comments and TODOs, but ultimately it's up to you.

One critical thing when doing so is that you communicate all the things that are happening in the PR description. You could also suggest alternatives that were considered (e.g. remove everything, remove the public API instead of deprecating it) to see if there's an appetite for those things.

@kcpevey
Copy link
Owner Author

kcpevey commented Apr 11, 2023

Sounds good. I did remove all the TODOs related to this work. I saw were there are a lot of TODOs in the octree files. Those were there before this work - not mine - so I left them alone.

I think I'll clean up the commentary in the config to add details on the env vars that were removed/changed and in what version (long term that commentary can be removed completely because ultimately it doesn't belong in the config anymore).

After that, I'll squash it all down and open up a clean PR to napari. And as you suggested, I'll spend time making a good summary and explanation in the PR description.

@kcpevey
Copy link
Owner Author

kcpevey commented Apr 21, 2023

This has now been opened on main napari#5711

@kcpevey kcpevey closed this Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.