-
-
Notifications
You must be signed in to change notification settings - Fork 8.3k
Unregister_scale() function to matplotlib.scale #31547
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
Draft
dikshajangra12918-oss
wants to merge
20
commits into
matplotlib:main
Choose a base branch
from
dikshajangra12918-oss:add-unregister-scale
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
12c9d66
Add unregister_scale() function to matplotlib.scale
dikshajangra12918-oss 9536088
Add tests for unregister_scale function
dikshajangra12918-oss 0a4417d
Use pop() instead of del for removing scale
cc686b1
Also remove from _scale_has_axis_parameter in unregister_scale
99690bc
Fix formatting in test_unregister_scale
d9bf8cf
Remove whitespace from blank line
2747f55
Update lib/matplotlib/scale.py
dikshajangra12918-oss e673532
Add deregister_scale to stub file
d56e2ef
Update tests to use deregister_scale
0081c89
Rename test functions to use deregister_scale
8400592
Replace all unregister_scale with deregister_scale
b83f45e
Use deregister_scale() in test cleanup
aaf5b7a
Remove outdated comment about deregister_scale
799bf8e
Fix TempScale using IdentityTransform
dc559da
Fix Whitespace in blank lines
41ded34
Remove outdated comments about no public deregister_scale()
5398c62
Rename deregister_scale to unregister_scale and protect built-in scales
390d67d
Fix whitespace and missing newline in test_scale.py
e0a87e8
Fix trailing blank line in test_scale.py
425d0cc
Add newline at end of test_scale.py
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -956,6 +956,25 @@ def register_scale(scale_class): | |||||||
| ) | ||||||||
|
|
||||||||
|
|
||||||||
| def unregister_scale(name): | ||||||||
| """ | ||||||||
| Remove a custom scale from the registry. | ||||||||
|
|
||||||||
| Parameters | ||||||||
| ---------- | ||||||||
| name : str | ||||||||
| The name of the scale to remove. | ||||||||
| """ | ||||||||
| _builtin_scales = {'linear', 'log', 'symlog', 'logit', | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if this isn't defined anywhere else, I think this should be defined up top and then added to the scale registery. Use the pattern from matplotlib/lib/matplotlib/cm.py Lines 195 to 197 in f6e850d
|
||||||||
| 'asinh', 'function', 'functionlog'} | ||||||||
| if name in _builtin_scales: | ||||||||
| raise ValueError( | ||||||||
| f"Cannot unregister built-in scale {name!r}.") | ||||||||
| if name not in _scale_mapping: | ||||||||
| raise ValueError(f"Scale '{name}' is not registered.") | ||||||||
| del _scale_mapping[name] | ||||||||
|
|
||||||||
|
|
||||||||
| def _get_scale_docs(): | ||||||||
| """ | ||||||||
| Helper function for generating docstrings related to scales. | ||||||||
|
|
||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if someone tries to deregister a built in scale? What should happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tangently I'm wondering if we should add a deregister function to the colormap registry (or is .
popgood enough since the registry itself is public?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@story645 the colormap registry has
https://matplotlib.org/stable/api/cm_api.html#matplotlib.cm.ColormapRegistry.unregister
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rcomer I think it should raise a "ValueError" to prevent accidental removal of built-in scales. I'll add that check to 'deregister_scale()'. Does that sound right?
@story645 That's good point! Should I address the colormap registry in this PR as well, or would it be better as a separate issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rcomer Thanks for the reference!
Following the same pattern as 'ColormapRegistry.unregister', I'll update 'deregister_scale()" to raise a 'ValueError' if someone tries to deregister a built-in scale like 'log', 'linear', etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @rcomer pointed out, the function already exists for colormaps. I just missed it in the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks for clarifying!