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

Only register if cmap not in colormaps #108

Merged
merged 3 commits into from
Jul 27, 2023
Merged

Conversation

hoxbro
Copy link
Member

@hoxbro hoxbro commented Mar 27, 2023

This comes up sometimes when debugging with panel serve --autoreload and changing a file in the holoviews/hvplot repo.

This has only come up for me when debugging, not actual use.

Steps to reproduce:

  1. Make a file called with import colorcet
  2. panel serve --autoreload that file
  3. Make some changes in colorcet/__init__.py and reload the page
ValueError: A colormap named "cet_diverging_isoluminant_cjm_75_c23" is already registered.

Traceback (most recent call last):
  File "/home/shh/miniconda3/envs/holoviz/lib/python3.10/site-packages/bokeh/application/handlers/code_runner.py", line 231, in run
    exec(self._code, module.__dict__)
  File "/home/shh/Development/holoviz/development/dev_hvplot/tmp.py", line 1, in <module>
    import colorcet  # noqa
  File "/home/shh/Repos/holoviz/colorcet/colorcet/__init__.py", line 551, in <module>
    m_diverging_isoluminant_cjm_75_c23 = mpl_cm('diverging_isoluminant_cjm_75_c23',diverging_isoluminant_cjm_75_c23)
  File "/home/shh/Repos/holoviz/colorcet/colorcet/__init__.py", line 92, in mpl_cm
    register_cmap("cet_"+name, cmap=cm[name])
  File "/home/shh/Repos/holoviz/colorcet/colorcet/__init__.py", line 70, in register_cmap
    colormaps.register(cmap, name=name)
  File "/home/shh/miniconda3/envs/holoviz/lib/python3.10/site-packages/matplotlib/cm.py", line 136, in register
    raise ValueError(
ValueError: A colormap named "cet_diverging_isoluminant_cjm_75_c23" is already registered.

@ianthomas23
Copy link
Member

It might be worth adding a new test to confirm that a ValueError is raised as in the example.

@hoxbro
Copy link
Member Author

hoxbro commented May 26, 2023

Added unit test. The failing tests on Python 3.6 are unrelated.

@jbednar
Copy link
Member

jbednar commented May 26, 2023

See #96 for issues with this approach. Can those be addressed?

@hoxbro
Copy link
Member Author

hoxbro commented May 26, 2023

I'm not sure I follow. Why is there an issue with this approach?

@jbednar
Copy link
Member

jbednar commented Jul 27, 2023

See #96 (comment) . Skipping registering the map is dangerous unless you verify that the map is indeed the same, not just the same name.

@jbednar
Copy link
Member

jbednar commented Jul 27, 2023

I'd be fine with skipping it if the name is the same and a Python equality test indicates that it's the same object, which should be true if it's just about reloading the same code. ETA: Looking again at the code, I now see a line that does check that it's the same object. In that case, this looks fine! Sorry for the noise!

@jbednar jbednar merged commit 0642f76 into main Jul 27, 2023
@jbednar jbednar deleted the only_register_cmap_once branch July 27, 2023 19:33
@jbednar
Copy link
Member

jbednar commented Jul 27, 2023

Python 3.6 tests were failing, which we'll have to consider when we do the next release. I assume that's to do with instantiating an environment rather than an actual problem, and I'm happy for this package to be installable on 3.6 even if there are aspects of the docs or other build steps that won't pass on 3.6.

@maximlt
Copy link
Member

maximlt commented Jul 29, 2023

That package won't be installable on Python 3.6 after the next release #110

@jbednar
Copy link
Member

jbednar commented Jul 30, 2023

Ah, yes. I guess it's fine; "colorcet" as a package should be available for Python 3.6, but the very latest versions need not be.

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.

4 participants