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

Cleanup builds #91

Merged
merged 3 commits into from
Aug 11, 2021
Merged

Cleanup builds #91

merged 3 commits into from
Aug 11, 2021

Conversation

joshmoore
Copy link
Member

After the extraction of the napari plugin to napari-ome-zarr,
a number of the matrix builds can be eliminated. This PR attempts
to drop any dependencies on Qt-related code which should all be in
the plugin repository.

After the extraction of the napari plugin to `napari-ome-zarr`,
a number of the matrix builds can be eliminated. This PR attempts
to drop any dependencies on Qt-related code which should all be in
the plugin repository.
@joshmoore
Copy link
Member Author

@will-moore : in order to get rid of the vispy dependency, can you see just adding these values raw rather than wrapping them with a Colormap?

                color = ch.get("color", None)
                if color is not None:
                    rgb = [(int(color[i : i + 2], 16) / 255) for i in range(0, 6, 2)]
                    # TODO: make this value an enumeration
                    if model == "greyscale":
                        rgb = [1, 1, 1]
                    colormaps.append(Colormap([[0, 0, 0], rgb]))

@will-moore
Copy link
Member

A Colormap was added here because that's what napari was expecting.

It looks like napari is still expecting a napari colormap in add_image: https://napari.org/docs/0.4.7/api/napari.Viewer.html#napari.Viewer.add_image but this won't be needed
now that napari support is moved to napari-ome-zarr.

@joshmoore
Copy link
Member Author

So store [[0, 0, 0], rgb] and then transform in napari-ome-zarr? Or introduce an object here?

@will-moore
Copy link
Member

I would think that [[0, 0, 0], rgb] would be fine, but happy to use an Object if it's clearer etc.

@codecov
Copy link

codecov bot commented Aug 10, 2021

Codecov Report

Merging #91 (d79c393) into master (68aca49) will increase coverage by 2.18%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #91      +/-   ##
==========================================
+ Coverage   67.18%   69.37%   +2.18%     
==========================================
  Files          10       11       +1     
  Lines         963     1035      +72     
==========================================
+ Hits          647      718      +71     
- Misses        316      317       +1     
Impacted Files Coverage Δ
ome_zarr/reader.py 58.16% <ø> (-0.22%) ⬇️
ome_zarr/utils.py 90.80% <0.00%> (-0.77%) ⬇️
ome_zarr/format.py 94.11% <0.00%> (ø)
ome_zarr/data.py 98.70% <0.00%> (+0.01%) ⬆️
ome_zarr/cli.py 88.60% <0.00%> (+0.14%) ⬆️
ome_zarr/writer.py 93.93% <0.00%> (+0.18%) ⬆️
ome_zarr/io.py 82.14% <0.00%> (+3.38%) ⬆️

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 68aca49...d79c393. Read the comment docs.

@joshmoore
Copy link
Member Author

joshmoore commented Aug 10, 2021

Failure is caused by the recently merged zarr-developers/zarr-python#781

Edit: fixed zarr's mainline.

Copy link
Contributor

@constantinpape constantinpape left a comment

Choose a reason for hiding this comment

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

@joshmoore this looks good. Could we merge this now and then merge main into #89 in order to simplify the tests?

@joshmoore
Copy link
Member Author

Yeah, let's do that. Then we can build up our confidence in this a bit more in this PR before cutting a release.

@joshmoore joshmoore merged commit a67ed4f into ome:master Aug 11, 2021
@joshmoore joshmoore deleted the cleanup-builds branch August 11, 2021 14:33
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.

3 participants