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

Fix label direct mode for installation without numba #6571

Merged
merged 8 commits into from Jan 8, 2024

Conversation

Czaki
Copy link
Collaborator

@Czaki Czaki commented Jan 4, 2024

Description

When working on fix bug in npe2 I spot that napari tests in npe2 repository are failing because of lack of numba.

In the context of one of the tests, it needs to be skipped when numba is not installed.

However, the second failure reveals a real bug in codebase.

This PR also modifies pip workflow to be used to test numbaless case.

@Czaki Czaki added the bugfix PR with bugfix label Jan 4, 2024
@Czaki Czaki added this to the 0.4.19 milestone Jan 4, 2024
@github-actions github-actions bot added tests Something related to our tests task labels Jan 4, 2024
Copy link

codecov bot commented Jan 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6253f42) 92.25% compared to head (7720b8a) 92.21%.
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6571      +/-   ##
==========================================
- Coverage   92.25%   92.21%   -0.05%     
==========================================
  Files         603      603              
  Lines       53814    53831      +17     
==========================================
- Hits        49648    49640       -8     
- Misses       4166     4191      +25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot added the qt Relates to qt label Jan 4, 2024
Copy link
Member

@jni jni left a comment

Choose a reason for hiding this comment

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

@Czaki I'm ok with this, but do you think we should test the new code?

napari/utils/colormaps/colormap.py Show resolved Hide resolved
max_value = max(
(abs(x) for x in self.color_dict if x is not None), default=0
)
if any(x < 0 for x in self.color_dict if x is not None):
Copy link
Member

Choose a reason for hiding this comment

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

We really need to deprecate None and use defaultdict I think... (next PR) Anyway, like below this isn't tested — would you like it to be?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is tested, but not with code coverage (in pip job). I will add additional tests

Copy link
Collaborator Author

@Czaki Czaki Jan 5, 2024

Choose a reason for hiding this comment

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

We do not want to use defaultdict globally. As defaultdict is adding elements when they are missed. So size is increasing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The option may be to add an additional property to DirectLAbelColormap to store default color outside color dict

Copy link
Member

Choose a reason for hiding this comment

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

Good point @Czaki, I like this option.

@jni jni merged commit bb785f3 into napari:main Jan 8, 2024
31 of 32 checks passed
@jni jni deleted the fix_tests_npe2 branch January 8, 2024 09:20
Czaki added a commit that referenced this pull request Jan 10, 2024
When working on fix bug in npe2 I spot that napari tests in npe2
repository are failing because of lack of numba.

In the context of one of the tests, it needs to be skipped when numba is
not installed.

However, the second failure reveals a real bug in codebase.

This PR also modifies `pip` workflow to be used to test numbaless case.
kne42 added a commit to kne42/napari that referenced this pull request Jan 11, 2024
* main:
  Fix labels mapping cache by filling it with background, not 0 (napari#6580)
  Simplify unused parameters of Quaternion functions. (napari#6424)
  Add size and ndim to LayerDataProtocol (napari#6494)
  Fix label direct mode for installation without numba (napari#6571)
  Fix test in napari_builtins to remove import from conftest (napari#6568)
  Remove `app-model!=0.2.4` from test constraints (napari#6577)
  Bump mypy version and fix errors (napari#6557)
  Update test to work with `app-model==0.2.4` (napari#6573)
  Added support for features in surface layers (napari#6515)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR with bugfix qt Relates to qt task tests Something related to our tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants