CDAT / cdat

Community Data Analysis Tools
Other
175 stars 68 forks source link

Discuss getting rid of boxfill() and similar method that duplicate plot() call #2021

Open aashish24 opened 8 years ago

aashish24 commented 8 years ago

Currently we can plot data using plot and boxfill() calls. I would prefer we just have one method for plotting unless there is a strong need for another method. Thoughts?

chaosphere2112 commented 8 years ago

@aashish24 Actually, we can plot data using plot, boxfill(), isofill(), isoline(), vector(), meshfill(), vector3d(), scalar3d(), dual_scalar3d() (:confused:) , oned(), xvsy(), xyvsy(), yxvsx(), and I probably missed a few. I'm pretty sure these are commonly used by our users as shortcuts for the whole getgraphicsmethod -> plot process, though we'd have to have @doutriaux1 chime in on that.

aashish24 commented 8 years ago

@aashish24 Actually, we can plot data using plot, boxfill(), isofill(), isoline(), vector(), meshfill(), vector3d(), scalar3d(), dual_scalar3d() () , oned(), xvsy(), xyvsy(), yxvsx()

Yup, I know. I was referring to boxfill() as an example (should have been more clear). But that's the thing, who uses these calls (none of our tests or other scripts I have seen using these). Most of them used plot().

I would like to favor smaller, simple, non-redundant API as much possible.

chaosphere2112 commented 8 years ago

I know that @doutriaux1 uses them extensively when tinkering on the command line :smile: I think this is the kind of thing we should try and get some metrics on. I really need to bring the analytics server back online; I'll try and clear some time to do that in the next week or two.

doutriaux1 commented 8 years ago

@chaosphere2112 I never use boxfill() and co. I actually strongly agree with @aashish24 on this one, we should get rid of them all, all they do anyway is call plot now if you want to get fancy and keep a similar approach, I would recommend actually implemeting a .plot() function on the graphic methods themselves.

import vcs
x=vcs.init()
gm = x.createboxfill()
gm.plot(data)
aashish24 commented 8 years ago

I would recommend actually implemeting a .plot() function on the graphic methods themselves.

:+1: that would nice since that will make the code compact.

createisofill().plot(data['clt']) # Create default canvas if none provided

There are two things in my mind really:

  1. Make vcs compact and lean from API point of view.
  2. Remove redundant calls which will help getting rid of lot of branch and duplicate code.
chaosphere2112 commented 8 years ago

I certainly agree that they add a lot of cruft. I guess we'd have to leave the canvas.createisofill() etc. and associate the created GM with the canvas for plot() on GMs to work, but that's not a big deal.

aashish24 commented 8 years ago

I certainly agree that they add a lot of cruft. I guess we'd have to leave the canvas.createisofill() etc. and associate the created GM with the canvas for plot() on GMs to work, but that's not a big deal.

Totally agree :smile:

danlipsa commented 8 years ago

@aashish24 @doutriaux1 +2 to using just a plot function. Moving plot on the gm from canvas is not a clear cut. Now you have

x=vcs.init() gm = x.createboxfill() x.plot(gm) x.clear()

which makes sense as well. Keeping this the same maintains a lot of the compatibility.

aashish24 commented 8 years ago

Okay, baby steps first:

  1. Announce to the list that we are getting rid of redundant plot calls alternative
  2. Remove these calls for 3.0
  3. Update documentation
doutriaux1 commented 8 years ago

@danlipsa I don't think we should get rid of x.plot() at all. Just add gm.plot() as a convenience function which would be better located than x.boxfill()

danlipsa commented 8 years ago

@doutriaux1 This reminds me of Perl vs Python. Perl: there is more then one way of doing it. Python: There should be one — and preferably only one — obvious way to do it. We know who won here :smile:

aashish24 commented 8 years ago

x=vcs.init() gm = x.createboxfill() x.plot(gm) x.clear()

@danlipsa look at this code. (based on @doutriaux1 suggestion)

import cdms2, vcs
data = cdms2.open('clt.nc')['clt']
vcs.init().createisofill(data).plot()

Done

I agree that duplicates are bad but since canvas and graphics methods are related and drawable entities it is okay to have plot method in both of them. For instance

vtkRenderWindow and vtkRender they both has Render() method.

In GeoJS

map, layers, and features all have draw() method.

aashish24 commented 8 years ago

Also you can do this now

import cdms2, vcs
data = cdms2.open('clt.nc')['clt']
vcs.init().plot(createisofill(), data)

The difference is subtle but it breaks the flow somewhat. Since isofill is connected to canvas anyways, you should be able to call plot on isofill and that should trigger the rendering too. I think it is a general use-cased and used in libraries like d3 and geojs quite a bit.

danlipsa commented 8 years ago

@doutriaux1 @aashish24 We can add that if you like it. :smile: It will be just a one line function anyway.

doutriaux1 commented 8 years ago

@aashish24 the only issue with your line is that it is really:

import vcs
x=vcs.init()
gm=x.createboxfill()
gm.plot(data)

And gm does not know about x object so it will likely create one under the hood, so your line actually creates a useless x and can be shortened/optimzed to:

import vcs
vcs.createboxfill().plot(data)
doutriaux1 commented 8 years ago

that actually brings the question, should gm.plot(data) return the canvas it used?

aashish24 commented 8 years ago

import vcs vcs.createboxfill().plot(data)

cool! Yes this would be preferred! Sounds good, I will add the plot method to graphics classes for 3.0.

chaosphere2112 commented 8 years ago

@doutriaux1 Above:

chaosphere2112 I guess we'd have to leave the canvas.createisofill() etc. and associate the created GM with the canvas for plot() on GMs to work, but that's not a big deal.

We can just do what vcsaddons does and add a canvas argument, so the canvas.create$GRAPHICSMETHOD functions will pass the canvas in as an additional arg to the GM.

aashish24 commented 8 years ago

hat actually brings the question, should gm.plot(data) return the canvas it used?

It should return the gm itself so that you can chain it for other calls too like in d3. I prefer it returns itself vs canvas since that would be bit off.

aashish24 commented 8 years ago

It should return the gm itself so that you can chain it for other calls too like in d3. I prefer it returns itself vs canvas since that would be bit off.

Actually I take it back. Thinking more into it, yes, it think it would be good if returns the canvas object.

doutriaux1 commented 8 years ago

@aashish24 yes I think so too, otherwise all the canvas are created and not reachable easily any longer.

import vcs
vcs.createboxfill().plot(data).plot(data,vcs.createisoline())