Fixed special cases of dir=90, dir=270 of wrapping dot field #163

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants
@alexholcombe
Contributor

alexholcombe commented Jun 27, 2012

Fixed special cases of dir=90, dir=270 of wrapping dot field (thanks Dani Linares)
BF, ENH

alexholcombe added some commits Jun 19, 2012

I implemented wrapping and it works now for all my tests. I will
now commit and request a pull.
To test it yourself, in the dots.py demo simply set dotLife=-1.
I don't think I nested the helper functions with correct style. I'm
sure there are also some other style violations due to my ignorance.
It wraps points one by one rather than being vectorized, so there must
be a speed penalty. I don't know that it can be vectorized. If it
can't and the speed penalty is judged significant, then we might need
to add a boolean parameter to dotStim to deactivate wrapping. However
it seems to work fast enough in my (limited) testing.
+ result = 0
+ discriminant = (a*b)**2 * (b**2 - n**2 + (a*m)**2)
+ if discriminant < 0:
+ numIntersetions=0;

This comment has been minimized.

@jeremygray

jeremygray Nov 18, 2012

Member

typo? numIntersetions

@jeremygray

jeremygray Nov 18, 2012

Member

typo? numIntersetions

This comment has been minimized.

@alexholcombe

alexholcombe Nov 18, 2012

Contributor

Yes you must be right! Unfortunately I am about to do all the semester's grading now so don't have time now to fix it. Thanks for finding this.
-Alex

On 19/11/2012, at 5:09 AM, "Jeremy R. Gray" notifications@github.com wrote:

In psychopy/visual.py:

  •                   numIntersections =1
    
  •                else:
    
  •                    numIntersections=2
    
  •                    ySquared = b_b_(1.0 - (x1_x1)/(a_a))
    
  •                    if ySquared < 0:
    
  •                        print 'point1=',point1,' point2=', point2, ' a=',a,' b=',b
    
  •                        raise Exception("Have miscomputed intersections of vertical line with ellipse")
    
  •                    y1 =  numpy.sqrt( ySquared )
    
  •                    y2 = -y1   #  -y1
    
  •            else: #not a vertical line so can use y = mx + n 
    
  •                m = dy/dx
    
  •                n = point1[1] - m*point1[0]
    
  •                result = 0
    
  •                discriminant = (a_b)__2 \* (b__2 - n__2 + (a_m)**2)
    
  •                if discriminant < 0:
    
  •                    numIntersetions=0;
    
    typo? numIntersetions


Reply to this email directly or view it on GitHub.

@alexholcombe

alexholcombe Nov 18, 2012

Contributor

Yes you must be right! Unfortunately I am about to do all the semester's grading now so don't have time now to fix it. Thanks for finding this.
-Alex

On 19/11/2012, at 5:09 AM, "Jeremy R. Gray" notifications@github.com wrote:

In psychopy/visual.py:

  •                   numIntersections =1
    
  •                else:
    
  •                    numIntersections=2
    
  •                    ySquared = b_b_(1.0 - (x1_x1)/(a_a))
    
  •                    if ySquared < 0:
    
  •                        print 'point1=',point1,' point2=', point2, ' a=',a,' b=',b
    
  •                        raise Exception("Have miscomputed intersections of vertical line with ellipse")
    
  •                    y1 =  numpy.sqrt( ySquared )
    
  •                    y2 = -y1   #  -y1
    
  •            else: #not a vertical line so can use y = mx + n 
    
  •                m = dy/dx
    
  •                n = point1[1] - m*point1[0]
    
  •                result = 0
    
  •                discriminant = (a_b)__2 \* (b__2 - n__2 + (a_m)**2)
    
  •                if discriminant < 0:
    
  •                    numIntersetions=0;
    
    typo? numIntersetions


Reply to this email directly or view it on GitHub.

@paulproteus

This comment has been minimized.

Show comment
Hide comment
@paulproteus

paulproteus Feb 16, 2013

I noticed that this pull request has become "stale" -- that is, it no longer applies against current origin/master.

So I wrote out the following little shell snippet (with explanations beginning with "#") that explains how to refresh it.

Anyone who wants to refresh the patch and fix it up would need to do these steps:

### To refresh https://github.com/psychopy/psychopy/pull/163
## Notice that it says alexholcombe wants to merge
## commits in from alexholcombe:master.
##
## That's a hint that the latest code in his personal fork
## (on the master branch) is probably equivalent.
# Add alex's repo
git clone git://github.com/alexholcombe/psychopy.git
cd psychopy
# Add just metadata
git remote add upstream git://github.com/psychopy/psychopy.git
# Download upstream code into a separate namespace
git fetch upstream
### Refresh the patch
git rebase upstream/master
### Eek! There are conflicts
git mergetool -t kdiff3
## (might require yum install kdiff3 or apt-get install kdiff3)
## kdiff3 finishes without even launching a GUI
## because it noticed it was just a whitespace conflict.
# So we carry on...
git rebase --continue
# And now we've refreshed the patches.
# You can see the June 2012 patches by Alex Holcombe
# on top of the latest stuff from Feb 2013.
git log

I noticed that this pull request has become "stale" -- that is, it no longer applies against current origin/master.

So I wrote out the following little shell snippet (with explanations beginning with "#") that explains how to refresh it.

Anyone who wants to refresh the patch and fix it up would need to do these steps:

### To refresh https://github.com/psychopy/psychopy/pull/163
## Notice that it says alexholcombe wants to merge
## commits in from alexholcombe:master.
##
## That's a hint that the latest code in his personal fork
## (on the master branch) is probably equivalent.
# Add alex's repo
git clone git://github.com/alexholcombe/psychopy.git
cd psychopy
# Add just metadata
git remote add upstream git://github.com/psychopy/psychopy.git
# Download upstream code into a separate namespace
git fetch upstream
### Refresh the patch
git rebase upstream/master
### Eek! There are conflicts
git mergetool -t kdiff3
## (might require yum install kdiff3 or apt-get install kdiff3)
## kdiff3 finishes without even launching a GUI
## because it noticed it was just a whitespace conflict.
# So we carry on...
git rebase --continue
# And now we've refreshed the patches.
# You can see the June 2012 patches by Alex Holcombe
# on top of the latest stuff from Feb 2013.
git log
@peircej

This comment has been minimized.

Show comment
Hide comment
@peircej

peircej Feb 16, 2013

Member

It wasn't pulled in at the time because I was worried about the potential performance hit of updating dots in a for-loop. I haven't got round to vectorising the code (if that's possible) to use numpy arrays so the pull request got kinda left in no-mans land.

Member

peircej commented Feb 16, 2013

It wasn't pulled in at the time because I was worried about the potential performance hit of updating dots in a for-loop. I haven't got round to vectorising the code (if that's possible) to use numpy arrays so the pull request got kinda left in no-mans land.

@alexholcombe

This comment has been minimized.

Show comment
Hide comment
@alexholcombe

alexholcombe Feb 17, 2013

Contributor

Yes that is definitely a well-justified concern. Thanks anyway to Paul for the preparation and instruction

On 17/02/2013, at 7:56 AM, Jon Peirce notifications@github.com wrote:

It wasn't pulled in at the time because I was worried about the potential performance hit of updating dots in a for-loop. I haven't got round to vectorising the code (if that's possible) to use numpy arrays so the pull request got kinda left in no-mans land.


Reply to this email directly or view it on GitHub.

Contributor

alexholcombe commented Feb 17, 2013

Yes that is definitely a well-justified concern. Thanks anyway to Paul for the preparation and instruction

On 17/02/2013, at 7:56 AM, Jon Peirce notifications@github.com wrote:

It wasn't pulled in at the time because I was worried about the potential performance hit of updating dots in a for-loop. I haven't got round to vectorising the code (if that's possible) to use numpy arrays so the pull request got kinda left in no-mans land.


Reply to this email directly or view it on GitHub.

@benjross

This comment has been minimized.

Show comment
Hide comment
@benjross

benjross Jun 29, 2013

Hi, I'm going to take a look at this

Hi, I'm going to take a look at this

@peircej peircej closed this Mar 16, 2014

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