Refactor: make rc_ultraplot and rc_matplotlib internal #384
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Note: I may just remove the inputs as the user should not be concerned with the defaults rc`s |
|
Still reaching the time-out ... |
beckermr
left a comment
beckermr
left a comment
There was a problem hiding this comment.
Is there a way we can do this without breaking the API?
| return kw_ultraplot | ||
|
|
||
|
|
||
| def config_inline_backend(fmt=None): |
There was a problem hiding this comment.
Why was this function removed?
There was a problem hiding this comment.
It is moved as a class function now. It was moved there as it was referencing as global var (rc_ultraplot). The encapsulation is more appropriate with this recent change.
There was a problem hiding this comment.
It is here now: https://github.com/Ultraplot/UltraPlot/pull/384/files#diff-849c439d454bf26e1102dce232b08a417344e1d97a4add022d4088cd00b743caR1060
Note this PR looks more heavy than it is. I moved _int and __init__ to the top of the class to make it read easier. The only major change is moving rc_ultraplot and rc_matplotlib as class variables and adjusting the logic elsewhere in the code (mainly a couple of references in figure.py)
There was a problem hiding this comment.
To add to this. I looked into to how this function is called, and it also happens inside Configurator. I first thought the function was called by IPython directly -- but it is not.
It doesn't change the api that much. Users are guided to use |
|
An API change is an API change even if it is small. We'll need to bump to 2.0 after this IMHO. |
|
I see a version increase merely as a version increase, but perhaps we should add an api warning for few Y releases (x.Y) before we make the change. |
| def sync(self): | ||
| """ | ||
| Sync settings between ultraplot and matplotlib rc dictionaries. | ||
| """ | ||
| for _src in (self.rc_ultraplot, self.rc_matplotlib): | ||
| for _key in _src: # loop through unsynced properties | ||
| if "color" not in _key: | ||
| continue | ||
| try: | ||
| _src[_key] = _src[_key] | ||
| except ValueError as err: | ||
| warnings._warn_ultraplot(f"Invalid user rc file setting: {err}") | ||
| _src[_key] = "black" # fill value |
There was a problem hiding this comment.
I don't follow how this code works to be honest. Also the method should probably be called sync_colors since it only deals with colors?
There was a problem hiding this comment.
Code was originally part of init. Merely wrapped it. I think it triggers the validators.
There was a problem hiding this comment.
I don't follow how this code works to be honest. Also the method should probably be called
sync_colorssince it only deals with colors?
actually taht is a good point -- will change it for clarity
There was a problem hiding this comment.
I don't see how the settings from one rc object end up in the other. It must happen on the backend as you say.
There was a problem hiding this comment.
At least for the rc_ultraplot it will run a validator on setting a key (see rcsetup.py); matplotlib prolly has a similar setting.
Initially I was also scratching my head at this section in the code. Will replace it as we progress to something a bit cleaner.
2 participants
This PR moves the global variables of
rc_ultraplotandrc_matplotlibinternal to theConfiguratorclass.Why?
Currently we have global variables that are being passed around which causes us to track
rc_matplotlib,rc_ultraplotandrcseparately. If we wanna move to a thread safercsetter ourConfiguratorclass needs to encapsulate the rc parameters properly. It eases our understanding of the how UltraPlot is configured without having to see where thercare update. The general use ofrcis focused on theConfiguratoranyway so making them apart of this setting is a logical encapsulation.It also moves the logic from
__init__.pyto a newsyncclass method.Todos
rcdicts insideConfigurator