Skip to content

Vectorize#2

Open
tylerapritchard wants to merge 19 commits into
christinahedges:vectorizefrom
tylerapritchard:vectorize
Open

Vectorize#2
tylerapritchard wants to merge 19 commits into
christinahedges:vectorizefrom
tylerapritchard:vectorize

Conversation

@tylerapritchard

Copy link
Copy Markdown

some vectorization updates

Comment thread src/tesspoint/tesspoint.py Outdated
return inView


def make_az_asym(coords):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I buy this let's ditch it! Thank you for the sanity check.

Comment thread src/tesspoint/tesspoint.py Outdated
cphi = np.cos(deg2rad * lng_deg)
sphi = np.sin(deg2rad * lng_deg)

xyfp = np.zeros((2,), dtype=np.double)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this line in Python

Comment thread src/tesspoint/tesspoint.py Outdated
sphi = np.sin(deg2rad * lng_deg)

xyfp = np.zeros((2,), dtype=np.double)
xyfp[0] = -cphi * rtanth

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
xyfp[0] = -cphi * rtanth
xyfp = [-cphi * rtanth, -sphi * rtanth]

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TP points out that we should maybe be thinking about type checking here and make sure it's a float.

Comment thread src/tesspoint/tesspoint.py Outdated

xyfp = np.zeros((2,), dtype=np.double)
xyfp[0] = -cphi * rtanth
xyfp[1] = -sphi * rtanth

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
xyfp[1] = -sphi * rtanth

Comment thread src/tesspoint/tesspoint.py Outdated
if xy[0] >= 0.0:
if xy[1] >= 0.0:
self.ccd = 1
ccdpx = xy_to_ccdpx(self, xy,)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ccdpx = xy_to_ccdpx(self, xy,)
ccdpx = xy_to_ccdpx(self, xy)

Comment thread src/tesspoint/tesspoint.py Outdated
"""Convert focal plane to pixel location also need to add in the
auxillary pixels added into FFIs
"""
#created xy_to_ccdpix function to minimize repeated math

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment here describing what we're trying to achieve with this function, and what xy is?

Comment thread src/tesspoint/tesspoint.py Outdated
lat = lat / deg2rad
cut = star_in_fov(lng, lat)
if(cut):
lng=lng[cut]

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
lng=lng[cut]
lng = lng[cut]

Comment thread src/tesspoint/tesspoint.py Outdated
lng = lng / deg2rad
lat = lat / deg2rad
cut = star_in_fov(lng, lat)
if(cut):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(cut):
if cut:

Comment thread src/tesspoint/tesspoint.py Outdated
ccdpx, fitpix = mm_to_pix(self,xyfp)
return inCamera, ccdNum, fitsxpos, fitsypos, ccdxpos, ccdypos
else:
print('No specified targets in Field of View')

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use logger or warnings instead of print.

@christinahedges

Copy link
Copy Markdown
Owner

Things to add:

  • Think about a changelog
  • np.pi * 180 --> np.rad2deg etc (I made this error too)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants