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

tickets/dm-7308 Port obs_sdss to Python 3 #23

Merged
merged 8 commits into from Nov 7, 2016
Merged

Conversation

fred3m
Copy link
Contributor

@fred3m fred3m commented Nov 3, 2016

No description provided.

Copy link
Contributor

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

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

A handful of short changes, otherwise looks good.

@@ -180,6 +180,8 @@ def getColumnNames():
"run rerun camcol field filter ra1 dec1 ra2 dec2 ra3 dec3 ra4 dec4".split() +
"strip psfWidth sky airmass quality isblacklisted".split()
)
def __hash__(self):
return id(self)
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth a one-line docstring along the lines of "necessary for hashing this object in python3." or somesuch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'll add it now

if npoints < 1:
raise RuntimeError("Cannot create scaling image. Found no fluxMag0s to interpolate")

x, z = zip(*sorted(zip(self._xList, self._scaleList)))
x, z = list(zip(*sorted(zip(self._xList, self._scaleList))))
Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh. I'm not happy about that line, but I guess we should just leave it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is one of those things with no great pythonic solution. If you have a better one I'd be happy to implement it, but unless I'm using numpy arrays I usually use this technique.

@@ -1,3 +1,5 @@
from builtins import str
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we not have the import str line in general, to avoid problems like happened with pipe_tasks? See if removing this breaks anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had forgotten about that. Thanks for the reminder

@@ -38,11 +41,13 @@
# NOTE: commented out to remove astropy.extern.six dependency
Copy link
Contributor

Choose a reason for hiding this comment

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

This commented block can be removed (along with the six.PY3 thing), since we now have py3 support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point

@@ -25,6 +25,8 @@
"""Test lsst.obs.sdss.selectFluxMag0Task and integration with lsst.obs.sdss.scaleSdssZeroPointTask
"""
from __future__ import print_function
from builtins import str
Copy link
Contributor

Choose a reason for hiding this comment

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

Another import str we can probably safely remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@@ -22,6 +22,9 @@
# see <http://www.lsstcorp.org/LegalNotices/>.
#
from __future__ import print_function
from builtins import str
Copy link
Contributor

Choose a reason for hiding this comment

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

Another import str.

@fred3m fred3m merged commit 918e393 into master Nov 7, 2016
@ktlim ktlim deleted the tickets/DM-7308 branch August 25, 2018 05:50
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.

None yet

2 participants