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

Optimize vtk #1038

Merged
merged 1 commit into from Feb 21, 2015

Conversation

Projects
None yet
5 participants
@doutriaux1
Member

doutriaux1 commented Feb 13, 2015

@aashish24 @dlonie @chaosphere2112 @williams13

This is the merge for animations

I still need input to add tests for animations, not sure how to go at it, but just added option to "preserve" png files at exit might be the way to go.

Lots of changes, so it's a big job to review.

@@ -74,7 +74,7 @@
import vcs.manageElements
class SIGNAL(object):

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

FWIW, it's generally a bad idea to mix large whitespace-only (or other maintenance-oriented) changes with actual code changes.

It makes it more difficult for reviewers to find the actual changes that need reviewing, and leads to odd VCS histories (for instance, changing non-VTK related files in a topic that is focused on cleaning up VTK usage).

This comment has been minimized.

@doutriaux1

doutriaux1 Feb 18, 2015

Member

i think these are due to our pre-commit that refuses to have any white space at the end of lines.... VERY annoying, so I believe I ran some sed on the files to get rid of all of them massively

#self.describe()
self.controller.animation_created = True
self.controller._unique_prefix=hashlib.sha1(time.asctime()+str(random.randint(0,10000))).hexdigest()
#print "NFRAMES:",self.controller.number_of_frames()

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

Leftover debugging statement

self.controller = controller
def run(self):
#self.describe()

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

Should be removed.

class VTKAnimationPlayback(animate_helper.AnimationPlayback):
def __init__(self, controller):
animate_helper.AnimationPlayback.__init__(self,controller)
pass

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

This shouldn't be needed (pass). If it is, please add a comment.

import atexit
atexit.register(self.close)
def draw_frame(self):
#print "Drawing frame:",self.frame_num,self._unique_prefix

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

More debugging leftovers

def draw_frame(self):
#print "Drawing frame:",self.frame_num,self._unique_prefix
png_name=os.path.join(os.environ["HOME"],".uvcdat",self._unique_prefix,"anim_%i.png" % self.frame_num)
if os.path.exists(png_name) and len(self.animation_files)==self.number_of_frames():# and self.playback_params.zoom_factor!=1:

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

Trailing dead code

continue # nothing to do
#Ok we have a slab, let's figure which slice it is
args=[]
N=1

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

Please don't use n and N in the same scope. This has lead to errors in other parts of the code in the past, and is a very bad practice. In general, one letter identifiers and identifiers that differ only in case aren't good ideas -- it can be hard to figure out what short identifiers are, and mixing cases is very error-prone.

N*=len(a)
args.append(slice(n,n+1))
args=args[::-1]
#if self.frame_num == 0:

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

Leftover debugging

else:
self.vcs_self.backend.update_input(disp.backend,slab(*args),slabs[1](*args),update=True)
self.vcs_self.backend.renWin.Render()
if not os.path.exists(png_name):

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

This is always true due to the outer if/else blocks.

@@ -415,11 +417,12 @@ def plot(self,data1,data2,template,gtype,gname,bg,*args,**kargs):
self._renderers[(None,None,None)]=ren
else:
ren = self._renderers[(None,None,None)]
vcs2vtk.genTextActor(ren,to=to,tt=tt)
returned["vtk_backend_text_actors"] = vcs2vtk.genTextActor(ren,to=to,tt=tt)

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

Is this map and its contents / expected keys / intended usage documented anywhere?

This comment has been minimized.

@doutriaux1

doutriaux1 Feb 18, 2015

Member

of course not! @chaosphere2112 when we do the doc for widgets we should also doc these since you're using them as well.

w = vcs2vtk.numpy_to_vtk_wrapper(w,deep=False)
w.SetName("vectors")
returned["vtk_backend_missing_mapper"]=missingMapper,None,False

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

What's the meaning of None and False here?

This comment has been minimized.

@doutriaux1

doutriaux1 Feb 18, 2015

Member

None is the color for missing, false is for cellData (True/False), these are the args sent before/later to: vcs2vtk.putMaskOnVTKGrid

if mapper is missingMapper:
actors.append([act,missingMapper,[x1,x2,y1,y2]])
else:
actors.append([act,[x1,x2,y1,y2]])

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

One path adds [actor, mapper, bounds], the other omits the mapper. Is this intentional?

This comment has been minimized.

@doutriaux1

doutriaux1 Feb 18, 2015

Member

yes only need 3 for missingMapper, I guess we could add None for other cases

@@ -1171,8 +1158,8 @@ def plotContinents(self,x1,x2,y1,y2,projection,wrap,tmpl):
contActor.GetProperty().SetColor(0.,0.,0.)
else:
geo=None
contMapper.SetResolveCoincidentTopologyPolygonOffsetParameters(1, 1)
contMapper.SetResolveCoincidentTopologyToPolygonOffset()
#contMapper.SetResolveCoincidentTopologyPolygonOffsetParameters(1, 1)

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

Please just remove, rather than commenting. We have the git history if we need to go back and revert things.

This comment has been minimized.

@doutriaux1

doutriaux1 Feb 18, 2015

Member

yes you added this one to get continents going but now it makes them disappear , any idea why? Didn't want to take it out completely before we are sure we don't need it.

for t in texts:
## ok we had a text actor, let's see if it's min/max/mean
txt = t.GetInput()
s0=txt.split()[0]

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

Is txtguaranteed to be non-empty? This will throw an exception if the text actor has an empty string.

This comment has been minimized.

@doutriaux1

doutriaux1 Feb 18, 2015

Member

I'd say yes but you're right I'll add a test for it just in case, it might actually be blank if some template has everything turned off.

This comment has been minimized.

@doutriaux1

doutriaux1 Feb 18, 2015

Member

Actually I take it back, it should always be non-empty. but you now better, canVTK have empty Input if we set string to ""

s0=txt.split()[0]
if s0 in ["Min","Max","Mean"]:
returned["vtk_backend_%s_text_actor" % s0] = t
self.canvas.display_names.remove(d.name)

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

This looks very out of place for rendering code. Should these lifetimes be managed elsewhere? If this is a one-off hack, it should have a comment.

This comment has been minimized.

@doutriaux1

doutriaux1 Feb 18, 2015

Member

it has to be here, render template created these display obejct so that they would be added to the returned, but that throws things off for @chaosphere2112 because now after the "plot" call we have 4 dispaly objects rather than one so they need to be removed. @chaosphere2112 is going to look into this as well and decides if he needs to return (and delete) more.

# if animate_helper.hasPyQt:
# self.signals.drew.emit()
def update_input(self,vtkobjects,array1,array2=None,update=True):

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

It's a good idea to put a blank line in between function definitions for readability.

def update_input(self,vtkobjects,array1,array2=None,update=True):
if vtkobjects.has_key("vtk_backend_grid"):
#print "ok?"

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

Leftover debugging.

data = vcs2vtk.numpy_to_vtk_wrapper(array1.filled(0.).flat, deep=False)
pData= vg.GetPointData().GetScalars()
if pData is not None:
#print "OK HERE"

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

Many more commented debug statements throughout this function

else:
missingMapper = None
if vtkobjects.has_key("vtk_backend_contours"):
for i,c in enumerate(vtkobjects["vtk_backend_contours"]):

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

Why enumerate? (i is not used)

This comment has been minimized.

@doutriaux1

doutriaux1 Feb 18, 2015

Member

used to be used, taking it off

@@ -35,6 +36,8 @@ class animate_obj_old(object):
##############################################################################
# Initialize the animation flags #
##############################################################################
def __del__(self):
print "Deleting the animation"

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

Probably don't want to leave this in.

def __init__(self):
threading.Thread.__init__(self)
self._stop = threading.Event()
self._running = threading.Event()
self._running.set()
def __del__(self):

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

Shouldn't be committed. Try using 'git gui' to prepare patches -- it lets you selectively add portions of the working tree's changes so you can commit real changes and leave debugging statements, etc out of the patch.

This comment has been minimized.

@doutriaux1

doutriaux1 Feb 18, 2015

Member

I thought I caught them all, but there's always some that escape!

This comment has been minimized.

@doutriaux1

doutriaux1 Feb 18, 2015

Member

idon't think we ever going in there anyway, we can delete the whole del

@@ -639,7 +653,7 @@ def run(self):
# this is how you allow the GUI to process events during
# animation creation
time.sleep(0.001)
#time.sleep(0.001)

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

Please delete if removing this.

This comment has been minimized.

@doutriaux1

doutriaux1 Feb 18, 2015

Member

I'm not positive it needs to be removed yet. @dakoop added this for the GUI otherwise it would freak out. But the GUI is broken right now so I can't test. Hence the "comment out" rather than delete.

@@ -660,7 +674,7 @@ def fps(self, value=None):
"""
if value is not None:
value = max(value, 0.5)
#value = max(value, 0.5)

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

Same.

This comment has been minimized.

@doutriaux1

doutriaux1 Feb 18, 2015

Member

let's put it back in

@@ -762,9 +782,11 @@ def is_playing(self):
return self.playback_running
def playback(self):
#print "CREATED:",self.created()

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

Shouldn't be committed.

if (self.created() and
(self.playback_thread is None or not self.playback_thread.is_alive())):
self.playback_thread = AnimationPlayback(self)
#print "Starting playback"

This comment has been minimized.

@allisonvacanti
if slabs[0] is None:
## Nothing to do
continue
#print "SLAB:",slabs[0].shape

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

More commented print statements added throughout this function.

@@ -931,6 +987,7 @@ def render_frame(self, frame_args, frame_num, frame_kargs=[]):
# and prevents segfaults when running multiple animations
#self.vcs_self.replot()
#print "*************************************************************"

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

Debug statement.

@@ -953,7 +1010,8 @@ def render_frame(self, frame_args, frame_num, frame_kargs=[]):
self.create_canvas.png(fn,draw_white_background=1)
return displays
def draw_frame(self, frame_num=None):
def draw_frame(self):
#print "drawing frame:",frame_num

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

Debug statement

@@ -1312,3 +1319,60 @@ def starPoints(radius_outer, x, y, number_points = 5):
theta += delta_theta
return points
def generateVectorArray(data1,data2,vtk_grid):

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

Needs newline before def

w = numpy_to_vtk_wrapper(w,deep=False)
w.SetName("vectors")
return w
def stripGrid(vtk_grid):

This comment has been minimized.

@allisonvacanti

allisonvacanti Feb 17, 2015

Contributor

Newline

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Feb 18, 2015

@dlonie addressed your comments. Thanks!

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Feb 18, 2015

@doutriaux1 there are quite a few of issues we found with this code. Let me schedule a tcon with you and @dlonie on Friday to discuss these.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Feb 18, 2015

@aashish24 @dlonie friday is code freeze, why wait so long?

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Feb 18, 2015

@dlonie @aashish24 what are the issues left, please post here or email me, I would rather not have to wait until the last minute to fix this.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Feb 19, 2015

@doutriaux1 this branch has lot of issues. We shouldn't merge this as it is. Let's talk sometime today.

missingMapper = None
if vtkobjects.has_key("vtk_backend_contours"):
for c in vtkobjects["vtk_backend_contours"]:
c.Update()

This comment has been minimized.

@aashish24

aashish24 Feb 19, 2015

Contributor

no need to call actors manually..

w = vcs2vtk.generateVectorArray(array1,array2,vg)
vg.GetPointData().AddArray(w)
vg = vcs2vtk.stripGrid(vg)
ports[0].SetInputData(vg)

This comment has been minimized.

@aashish24

aashish24 Feb 19, 2015

Contributor

Call SetInputConnection?

This comment has been minimized.

@doutriaux1

doutriaux1 Feb 19, 2015

Member

I didn't write that bit originally.

j+=1
#print "renderer:",i,"actor",j
m = actor.GetMapper()
m.Update()

This comment has been minimized.

@aashish24

aashish24 Feb 19, 2015

Contributor

Don't call Update on mapper like this.

This comment has been minimized.

@doutriaux1

doutriaux1 Feb 19, 2015

Member

I think you taught me this

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented Feb 19, 2015

@aashish24 @doutriaux1 Can I sit in on that call? I'm dependent on this branch for the VCS2D interactivity stuff I'm working on– the actors that Charles exposes are used to skip all of the previously used canvas.update() calls.

def draw_frame(self):
png_name=os.path.join(os.environ["HOME"],".uvcdat",self._unique_prefix,"anim_%i.png" % self.frame_num)
if os.path.exists(png_name) and len(self.animation_files)==self.number_of_frames():
## Ok we have the pngs and we need to zoom, need to use png

This comment has been minimized.

@chaosphere2112

chaosphere2112 Feb 19, 2015

Contributor

Can we get an option to require that we don't use PNGs? This will help a lot with scrubbing the animation from the VCS2D interactive code.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Feb 19, 2015

@aashish24 trying to reach you but can't get through either work or cell phone

Merge pull request #3 from chaosphere2112/optimize_vtk
draw_frame specify frame #
@allisonvacanti

This comment has been minimized.

Contributor

allisonvacanti commented Feb 20, 2015

Two failures, but I believe these were fixed in a recent patch (IIRC the filenames were incorrect).

https://www.cdash.org/viewTest.php?onlyfailed&buildid=3702821

LGTM -- @doutriaux1 let me know if those failures are ok and I'll merge.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Feb 20, 2015

@dlonie @aashish24 fix for these tests is already in another PR see commit 18709de

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented Feb 21, 2015

I'm gonna go ahead and merge this

@chaosphere2112 chaosphere2112 merged commit 84bdabb into CDAT:master Feb 21, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

chaosphere2112 added a commit that referenced this pull request Feb 21, 2015

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Feb 21, 2015

all right little beetle! I like that move! Do you feel the power rush?

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented Feb 21, 2015

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Feb 21, 2015

apparently in english it's not "little beetle" but "grasshopper"
kung-fu_tv-master_po-young_grasshopper

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented Feb 21, 2015

But at least je n'ai pas un chat dans la gorge!

(High school french left me with a number of weird colloquialisms ingrained into my mind)

@doutriaux1 doutriaux1 deleted the doutriaux1:optimize_vtk branch Mar 4, 2015

@coveralls

This comment has been minimized.

coveralls commented Sep 23, 2016

Coverage Status

Changes Unknown when pulling 84bdabb on doutriaux1:optimize_vtk into * on UV-CDAT:master*.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment