Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

iosonofabio
Copy link

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.)

@iosonofabio iosonofabio mentioned this pull request Nov 5, 2014
name = 'linear'

def __init__(self, axis, **kwargs):
Copy link
Member

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.

Copy link
Author

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...

@tacaswell
Copy link
Member

Looking at this code, the reason that LogScale and friends use kwargs and get passed in an axis is sort out what the names should be.

This whole section of code looks like it could use some simplification.

  • remove Axis.set_scale (as it has been deprecated)
  • remove axis as a positional arg to all of the scales and explictly put base, nonpos, ... as keywords. They should still collect kwargs, do the conversion, and raise a warning if users are passing in nonpos{x,y} and such
  • re-write the factory
  • modify Axes.set_{x,y}scale to use the new kwargs and to capture, clean, and raise a warning if the user passes in the old kwargs
  • verify all of this is tested / add tests the expect warnings
  • document all of this

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).

@iosonofabio
Copy link
Author

alright, I'll give it a shot but it's going to take me a few days.
F

@tacaswell
Copy link
Member

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.

@iosonofabio
Copy link
Author

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 axis.get_minpos but no axis.get_maxpos), I guess if we update the API one could have a look at those too.

@mdboom
Copy link
Member

mdboom commented Nov 6, 2014

axis.get_minpos returns the minimum positive number, and it is used by various log scales to figure out which lowest "decade" to display. axis.get_maxpos (which I don't advocate adding) would be identical to get_data_interval[1] anyway.

@iosonofabio
Copy link
Author

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?

@mdboom
Copy link
Member

mdboom commented Nov 7, 2014

Yeah -- smallest positive is very specific to log scales. I didn't quite understand what you were proposing with get_maxpos -- it may be useful to also track the largest positive value under 1, but you're right we don't have it yet. Doing that will require digging down in the C++ layer where get_maxpos is currently calculated.

@iosonofabio
Copy link
Author

Yes, I was very unclear 😃 sorry. I would need the maximal number below one for the same reason like LogScale needs the minimal positive, but I was afraid of touching the API. If you think I can go ahead though, I'll just get a C++ shovel and add a little function.

@tacaswell
Copy link
Member

Couldn't this be done in the python layer with a couple of np.clip calls on the data interval?

@tacaswell tacaswell modified the milestones: proposed next point release, next point release Feb 19, 2015
@tacaswell tacaswell modified the milestones: proposed next point release, next point release Jun 23, 2015
@tacaswell
Copy link
Member

Closing due to inactivity.

@iosonofabio If you still want to work on this ping to have this re-opened!

@tacaswell tacaswell closed this Mar 22, 2016
@QuLogic QuLogic modified the milestones: unassigned, 2.1 (next point release) Mar 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants