Skip to content
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

Isoline line, linecolors, and linewidths behave oddly #1944

Closed
chaosphere2112 opened this issue Apr 21, 2016 · 17 comments
Closed

Isoline line, linecolors, and linewidths behave oddly #1944

chaosphere2112 opened this issue Apr 21, 2016 · 17 comments
Assignees
Milestone

Comments

@chaosphere2112
Copy link
Contributor

chaosphere2112 commented Apr 21, 2016

So, there's some weirdness in the way we set up isoline objects.

IPython 4.1.2 -- An enhanced Interactive Python.
?         -> Introduction and overview of IPython's features.
%quickref -> Quick reference.
help      -> Python's own help system.
object?   -> Details about 'object', use 'object??' for extra details.

In [1]: import vcs
i
In [2]: isoline = vcs.createisoline()

If we do a .list(), we see our attributes:

In [3]: isoline.list()
 ----------Isoline (Gi) member (attribute) listings ----------
...
line =  ['solid']
linecolors =  [1]
linewidths =  [1.0]
...

If we set .line to the names of some line objects, some odd, but understandable behavior occurs:

In [4]: isoline.line = ["blue", "red"]

In [5]: isoline.list()
 ----------Isoline (Gi) member (attribute) listings ----------
...
line =  ['dash', 'dash']
linecolors =  [244, 242]
linewidths =  [2, 2]
...

It harvests the type attribute from the blue and red line objects, and sets those in line. It grabs the color attribute from each of them and sets linecolors. It grabs the width attribute and sets linewidths. All perfectly fine.

Now let's watch the world burn.

In [6]: isoline.line = ["green", "black"]

In [7]: isoline.list()
 ----------Isoline (Gi) member (attribute) listings ----------
...
line =  ['dash', 'dash']
linecolors =  [244, 242]
linewidths =  [2, 2]
...

Looks like... linecolors is exactly the same? Huh?

I'll make my own line object just to drive the point home:

In [8]: myline.list()
 ----------Line (Tl) member (attribute) listings ----------
secondary method = Tl
name = __line_787653798724505
type = ['dot']
width = [10]
color = [(0.0, 0.0, 100.0, 100.0)]
priority = 1
viewport = [0.0, 1.0, 0.0, 1.0]
worldcoordinate = [0.0, 1.0, 0.0, 1.0]
x = None
y = None
projection = default
colormap = None

I've set width, type, and color to be different from what was in isoline's attributes...

In [14]: isoline.line = [myline, myline]

In [15]: isoline.list()
 ----------Isoline (Gi) member (attribute) listings ----------
...
line =  ['__line_787653798724505', '__line_787653798724505']
linecolors =  [244, 242]
linewidths =  [2, 2]
...

OK, that's an extra bit of weird. Seems like instead of pulling the line's type, it pulled the line object's name... and still didn't update linecolors or linewidths.

Aside: Surprisingly, when plotted, this doesn't throw an error that __line_787653798724505 isn't a valid line type; that's because when we plot, we create a graphics method that inherits from the one that is passed in, which grabs the line name and then does the above described behavior (loading the style, though not the color or width.

These issues can be traced back to the isoline module, and the behaviors of the _setline function. I think what happened is that at some point the default value of linecolors and linewidths changed from None to [1], which makes it so it only replaces those values if you set lines to a list that has more elements than you have in linecolors or linewidths. We should definitely alter that behavior; I think we probably should go to None triggering default values, rather than automatically filling in the defaults (that way we can "know" if a user has specified data, which we shouldn't automatically override when we set the line attribute).

There's one more issue, which is what happens when you do this:

In[16]: import cdms2

In[17]: f = cdms2.open(vcs.sample_data + '/clt.nc')

In[18]: s = f("clt")

In[19]: isoline.line = ["dash-dot"]

In[20]: isoline.linecolors = [250]

In[21]: isoline.linewidths = [25]

In[22]: x = vcs.init()

In[23]: x.plot(s, isoline)

which generates the below image:

no_fat_level

As you can see, none of those lines have a width of 25, or a style of "dash-dot". That's because the "first" level is something like [0, 0] for the isoline, which is a very small chunk of the data, so it doesn't get displayed by VTK. We then pad out values for the rest of the levels, but we don't follow normal behavior for VCS. Every other place in VCS, when we pad out a list, we use the last value in the list, but in Isoline, we're using "solid" for line and 1.0 for linewidths (though we are using the last color for linecolors). This can be fixed in isofillpipeline by replacing the default values.

@danlipsa
Copy link
Contributor

@chaosphere2112 Why do you try to set a 'line object' such as 'blue' or 'red' to a place that should contain the 'line style' such as 'solid', 'dash', ...
I agree that his should report an error. Is this what you want?

@chaosphere2112
Copy link
Contributor Author

Nope, it shouldn't be an error (since the attribute is named "line", making it an error when you pass a line seems a bit rude).

I'll get back to you more in depth later, I've got other stuff I'm working on.

@danlipsa
Copy link
Contributor

@chaosphere2112 The documentations says (isoline.py:297):

Specify the isoline line style (or type):
iso.line=([0,1,2,3,4]) # Same as
iso.line=(['solid, 'dash', 'dot', 'dash-dot', 'long-dash']), will
specify the isoline style

To me that means you should set there a line style not a line object. I think we should follow the documentation and only allow line styles (report an error if we try to set a line object). We could change the fileld name in the future if we want.

@danlipsa
Copy link
Contributor

@chaosphere2112 grepping through the vcs sources seems to show the same usage: we set line styles not line objects.

@doutriaux1
Copy link
Contributor

@danlipsa it's supposed to accept a line object though. And use whatever line type is set on the object.

@danlipsa
Copy link
Contributor

@doutriaux1 Can we fix this? This is confusing and error prone. What if you set:
iso.line = ['green'] # line object
iso.linecolors = ['red']

Are you going to have red or green as the line color?

@aashish24
Copy link
Contributor

Hey guys, so here is my suggestion: So the isoline object has these usable properties:

'angle',
 'clockwise',
 'colormap',
 'datawc',
 'datawc_calendar',
 'datawc_timeunits',
 'datawc_x1',
 'datawc_x2',
 'datawc_y1',
 'datawc_y2',
 'g_name',
 'info',
 'label',
 'labelbackgroundcolors',
 'labelbackgroundopacities',
 'labelskipdistance',
 'level',
 'levels',
 'line',
 'linecolors',
 'linewidths',
 'list',
 'name',
 'projection',
 'scale',
 'script',
 'spacing',
 'text',
 'textcolors',
 'xaxisconvert',
 'xmtics',
 'xmtics1',
 'xmtics2',
 'xticlabels',
 'xticlabels1',
 'xticlabels2',
 'xyscale',
 'yaxisconvert',
 'ymtics',
 'ymtics1',
 'ymtics2',
 'yticlabels',
 'yticlabels1',
 'yticlabels2'

Whereas the line object has these:

'color',
 'colormap',
 'list',
 'name',
 'priority',
 'projection',
 's_name',
 'script',
 'type',
 'viewport',
 'width',
 'worldcoordinate',
 'x',
 'y'

Currently for most part, we have isoline which is a superset of line attributes (some has different names). I would suggest that

  • isoline: We change the name of line to linepatterns or linestyles
  • Do not use line object as it could be confusing. We have confusing API with unclear benefit.
  • We should look into the line API more carefully.

May be we can talk about it tomorrow but I prefer to keep the API simple unless there is a strong reason behind multiple ways of setting things up.

@aashish24
Copy link
Contributor

aashish24 commented May 31, 2016

what I would suggest is to create a linestyle object and then in the future we can use that for line and isoline. For instance

ls = vcs.createlinestyle(color="red", pattern="dash")

// here ls does not have any worldcoordinate, x, y, etc

iso = vcs.createisoline()
iso.linestyle(ls)

ln = vcs.createline()
ln.linestyle(ls)

@doutriaux1
Copy link
Contributor

@danlipsa the line attribute is what we're talking about it here, so green could only be a line name here, not the color.

@danlipsa
Copy link
Contributor

@doutriaux1 So 'green' refers to a line object, isn't it?
So you set the isoline color thought both the line object and the linecolor attribute. Which one wins?

@doutriaux1
Copy link
Contributor

whichever was set last. The idea is that you use line and then tweak some more if you want to change just one thing (color or width or type)

@danlipsa
Copy link
Contributor

danlipsa commented May 31, 2016

@doutriaux1 Why do we use a field ('line' = line style) set function to set several fields (line style, color and width)? I would say it would be a lot cleaner to have a separate function such as SetAttributesFromLine. What do you think?

@aashish24
Copy link
Contributor

@doutriaux1 @danlipsa I don't think we should support passing the line style via line object. As a developer I think its very confusing and could be confusing for the user too. Currently the line object contains 'world coordinates' and viewport. What happens to these when we pass line to isoline?

I agree with @danlipsa to have a method line CopyStyleFromLine if we want to support it but in general I think we should closely look at the API.

@danlipsa
Copy link
Contributor

danlipsa commented Jun 2, 2016

@doutriaux1 @aashish24 Is the consensus that we should support copying of certain line attributes (style, color, width) to isoline through a function: CopyLineAttributes?

@doutriaux1
Copy link
Contributor

@aashish24 the following already works and is used by scientists here:

iso= x.createisoline()
l=x.createline("aashish")
l.type = "dot"
l.width=10
l.color=["red"]
iso.line = ["dot",l]
iso.list()

There are some inconsistencies though the following will not work:

l.color="red"
iso.line = ["dot","aashish"]

@doutriaux1
Copy link
Contributor

@aashish24 also the texttable objects do have worldcoordinate and viewport attributes are are being used by template objects even though these attributes are not needed by the template. Although we could revise that and take viewport/worldcoordinate out of texttable and put it only on textcombined objects

@danlipsa
Copy link
Contributor

danlipsa commented Jun 2, 2016

Partially solved by #2005.
See the new image generated
https://github.com/UV-CDAT/uvcdat-testdata/pull/139/files

danlipsa added a commit that referenced this issue Jul 6, 2016
Add separate function to set type, color and width from a line.
danlipsa added a commit that referenced this issue Jul 6, 2016
Add separate function to set type, color and width from a line.
danlipsa added a commit that referenced this issue Jul 6, 2016
Add separate function to set type, color and width from a line.
danlipsa added a commit that referenced this issue Jul 26, 2016
Add separate function to set type, color and width from a line.
danlipsa added a commit that referenced this issue Jul 26, 2016
Add separate function to set type, color and width from a line.
danlipsa added a commit that referenced this issue Jul 27, 2016
Add separate function to set type, color and width from a line.
danlipsa added a commit that referenced this issue Aug 3, 2016
Add separate function to set type, color and width from a line.
danlipsa added a commit that referenced this issue Aug 3, 2016
Add separate function to set type, color and width from a line.
danlipsa added a commit that referenced this issue Aug 4, 2016
Add separate function to set type, color and width from a line.
danlipsa added a commit that referenced this issue Aug 5, 2016
Add separate function to set type, color and width from a line.
doutriaux1 pushed a commit that referenced this issue Aug 10, 2016
…#2046)

* BUG #1944: Rename line to linetype for isoline, unified1d and vector.

Add separate function to set type, color and width from a line.

* Fix test_vcs_read_old_scr_2 - we have one less template
chaosphere2112 pushed a commit that referenced this issue Sep 1, 2016
…#2046)

* BUG #1944: Rename line to linetype for isoline, unified1d and vector.

Add separate function to set type, color and width from a line.

* Fix test_vcs_read_old_scr_2 - we have one less template
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants