-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
scale: little API change in linear scale #3756
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
Conversation
name = 'linear' | ||
|
||
def __init__(self, axis, **kwargs): |
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.
We can't drop the init function entirely due to the scale_factory
function at the bottom of this module.
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.
wait: why don't we include this dummy constructor in the ScaleBase
class then? Every to-be-registered scale needs to accept those arguments because of the scale_factory
function...
Looking at this code, the reason that This whole section of code looks like it could use some simplification.
I think that is 'all' that needs to be done, but I only spent a bit of time look at this. I suspect that the consequences will be a bit more wide ranging (because they always are). |
alright, I'll give it a shot but it's going to take me a few days. |
Thanks! Before you spend too much time on this, I would like to get feed back from @mdboom @efiring @pelson @WeatherGod on if this should be done. I am in favor as it simplifies/unifies the api and (eventually after we drop the back-compatibility logic) greatly simplifies the code to reduce maintenance burden. |
Alright. Btw, when I was coding the logit scale today, I realized that there are a few little bugs that make the API not quite clean (e.g. there is an |
|
Thanks. I saw it's the smallest positive, but that's quite ad-hoc, isn't it? I mean, for the Logit scale I would need the maximal number strictly less than one, but I guess we are not adding one? |
Yeah -- smallest positive is very specific to log scales. I didn't quite understand what you were proposing with |
Yes, I was very unclear 😃 sorry. I would need the maximal number below one for the same reason like |
Couldn't this be done in the python layer with a couple of |
Closing due to inactivity. @iosonofabio If you still want to work on this ping to have this re-opened! |
Follow-up of PR #3753. The linear scale, LinearScale, has an empty constructor that swallows **kwargs. I propose to dump it.
(LogScale and its symmetric twin are a different case, because they use **kwargs to discriminate x axes from y axes.)