Add bermuda backend for faster triangulation
#7747
Conversation
|
The Qt benchmark run requested by PR #7747 (d7ed4e2 vs 6f702fe) has finished with status 'cancelled'. See the CI logs and artifacts for further details. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7747 +/- ##
==========================================
- Coverage 92.98% 92.93% -0.05%
==========================================
Files 635 637 +2
Lines 59857 60023 +166
==========================================
+ Hits 55656 55784 +128
- Misses 4201 4239 +38 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@Czaki are we sure we want to replace rather than allow user-selectable backends? Currently we have:
Kinda crazy but I'm pretty proud of how much experimentation we've done in a short time. 😃 It's plausible that different libraries have different bottlenecks, and different crash edge cases. In that case it might be useful for users to choose which backend they want to use? The major downside to my proposal is that we would need to update the preferences schema, which might indeed not be worth the trouble. Let me know what you're thinking — I can be convinced to drop PartSegCore-compiled-backend altogether if you prefer that. |
|
When this PR is ready to merge, the Bermuda should support all cases supported by PartSegCore (I need to investigate current crashes). I see the advantage of allowing to enable/disable triangle, but not to see selection between pure python and numba. But if you think it is worth, it could be done. |
|
The Qt benchmark run requested by PR #7747 (3a3f01b vs 6f702fe) has finished with status 'success'. See the CI logs and artifacts for further details.
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY. |
|
I have skipped the example that crashes benchmarks to measure performance improvements. Once #7766 is merged (need to wait on CI) I will update this PR and revert the last commit to debug what happened to fix the bug in bermuda. |
Well, the parentheses say it all, I think! 😂 Although the two backends are supposed to be equivalent, differences in syntax, compilers, and maybe even implementation bugs may cause crashes in different circumstances, and rapidly switching between backends (without having to modify the environment) could be helpful in debugging? 🤷 But, this could always be added in 0.6.1 so that we have time to figure out the settings schema migration. |
|
... Although, as I write that: for users, it is probably a good idea to actually do the proper migration: if you already have it turned on and you have partsegcore-compiled-backend installed and you upgrade to 0.6.0 in the same environment, it would be quite annoying to have your shapes layers suddenly be slow again. With a migration, you could change True to partsegcore-compiled-backend, and then it would allow people to do in-place upgrades without this issue... (We could leave numba/triangle/none for a future release.) (btw, the reason to run the pure Python backend on demand is that sometimes you encounter bad data that causes crashes in the compiled backends or in numba but that are easier to diagnose in pure Python.) |
|
The Qt benchmark run requested by PR #7747 (deda7e5 vs 6f702fe) has finished with status 'success'. See the CI logs and artifacts for further details.
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY. |
# References and relevant issues For easier debugging of #7747 # Description In #7632 I have added dump of problematic shapes that crash triangulation. I think that such an option to debug problematic cases may be introduced in other part of the codebase. This PR adds to benchmarks an upload of this data dump for easier debugging of CI.
|
The Qt benchmark run requested by PR #7747 (64b6ccb vs 4362250) has finished with status 'success'. See the CI logs and artifacts for further details. |
|
I also expected that it will not work. My opinion is that we should calculate default based on environment based on environment, to provide a user the fastest solution by default. We may name it |
ha, cool stuff @Czaki! I would like to get this in quickly, though, so I think what I will do is add back |
And hide from interface? |
I would like this but is there an easy way to do so? For now it is at the bottom and described as unused, pointing to the correct setting.
Do you have time to work on this now? I need to make dinner soon but will come back to this afterwards. |
|
Hmmm, even after using the default setting in the migration, I am still getting this failure: _________________________ test_env_settings_restore _________________________
monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x156dd7a40>
def test_env_settings_restore(monkeypatch):
monkeypatch.setenv('NAPARI_ASYNC', '0')
s = NapariSettings()
s.experimental.completion_radius = 1
assert s.env_settings() == {'experimental': {'async_': '0'}}
> assert s._save_dict()['experimental'] == {'completion_radius': 1}
E AssertionError: assert {'completion_...: pure_python} == {'completion_radius': 1}
E
E Omitting 1 identical items, use -vv to show
E Left contains 1 more item:
E {'triangulation_backend': pure_python}
E Use -v to get more diff
napari/settings/_tests/test_settings.py:436: AssertionError |
It was caused by changes in But if we set |
|
Tested this locally and it's working great! |
It happens when use |
Oh. I saw in one of the test failures that here: https://github.com/napari/napari/actions/runs/14595824772/job/40941394457?pr=7747#step:14:485
Does this mean that one would need to write |
If this is the case, I am opposed to this change... |
add `_missing_` classmethod
| return self.name.replace('_', ' ').title() | ||
| return str(self.value) |
There was a problem hiding this comment.
This seems like a step backward. I think it should be reverted, and it suggests to me that the changes from auto() were not needed...
There was a problem hiding this comment.
These changes comes from this line in preference dialog code:
So instead of using str(enum) we use enum.value for preparing dialog. I do not want to touch this in this PR...
There was a problem hiding this comment.
oooh, I thought this was in StrEnum, not in TriangulationBackend! Ok this is fine. 🙏
|
This issue has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/upcoming-napari-0-6-0-release/108954/10 |
|
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: |
References and relevant issues
Description
Add
bermudaas next backend for polygon triangulation.Introduce mechanism for selecting triangulation backend at runtime.