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

move colormaps from layers folder to resources folder #156

Merged
merged 2 commits into from
Mar 5, 2019

Conversation

sofroniewn
Copy link
Contributor

@sofroniewn sofroniewn commented Mar 4, 2019

Description

Resolves #152.

Type of change

  • Bug-fix (non-breaking change which fixes an issue)

References

How has this been tested?

  • run examples scripts

Final checklist:

  • My PR is the minimum possible work for the desired functionality

@sofroniewn sofroniewn added the refactor Refactoring code base label Mar 4, 2019
@sofroniewn sofroniewn requested a review from a team March 4, 2019 02:15
Copy link
Member

@kne42 kne42 left a comment

Choose a reason for hiding this comment

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

LGTM! Don't forget to change MANIFEST.in. While you're at it, please also rename all instances of napari_gui to napari in there :)

@royerloic royerloic self-requested a review March 4, 2019 23:24
Copy link
Member

@royerloic royerloic left a comment

Choose a reason for hiding this comment

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

Ressource folders are almost always reserved for non-code files such as icons, images, text files, etc...
I see a lot of python code going into the ressource folder, which i am not sure makes sense.
I am perfectly happy with colormaps living in a napari.colormaps package.

What would make sense to move to ressources is any text file (non .py) that would describe colormaps, if there is such a thing...

@sofroniewn
Copy link
Contributor Author

Ok, that sounds reasonable - does @jni want to weigh in before I move anything again?

@jni
Copy link
Member

jni commented Mar 5, 2019

He does not. =P But he agrees with @royerloic's general gist. =)

@sofroniewn
Copy link
Contributor Author

@royerloic how about a colormaps folder inside the util folder?

@sofroniewn
Copy link
Contributor Author

Have now moved to util folder. If @royerloic approves and tests pass he can merge

@royerloic
Copy link
Member

You are going to hate me...
I would simply have the colormaps in:
napari.colormaps

util packages usually contain utility methods and classes used internally multiple times.
If we forsee people adding colormaps by invoking classes or methods from this colormaps
package, I would really expose it as 'napari.colormaps'.

It is not a big deal at this point, we can revisit that later.

@royerloic royerloic merged commit 4d71357 into napari:master Mar 5, 2019
@kne42
Copy link
Member

kne42 commented Mar 5, 2019

IMO seeing as how these colormaps are specific to image layers, they should live in th3 _image_layer folder. Users who want to add more should add a new entry to Image.colormaps.

@jni
Copy link
Member

jni commented Mar 5, 2019

Totally possible to colormap points, lines, shapes... =)

@jni
Copy link
Member

jni commented Mar 5, 2019

(even desirable)

@sofroniewn
Copy link
Contributor Author

These are all good points, we also need to curate the list of colors available for the marker edge_color and face_color which right now is a giant list. We might want to that inside a napari.colors or napari.colormaps too.

I will leave this PR for now, but we can revisit when we address some of our other color related issues like #97

We might also want to look at the organisation of util again too, including the misc.py which is a bit of a grab bag right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactoring code base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move vendored colormaps to resources folder
4 participants