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

python3 port Tickets/dm 7288 #42

Merged
merged 5 commits into from Sep 15, 2016
Merged

python3 port Tickets/dm 7288 #42

merged 5 commits into from Sep 15, 2016

Conversation

parejkoj
Copy link
Contributor

No description provided.

Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

Looks pretty straightforward.

print >> fd, s.getId(), s.getXAstrom(), s.getYAstrom(), s.getRa(
), s.getDec(), s.getPsfFlux(), s.getFlagForDetection()
print(s.getId(), s.getXAstrom(), s.getYAstrom(), s.getRa(
), s.getDec(), s.getPsfFlux(), s.getFlagForDetection(), file=fd)
Copy link
Member

Choose a reason for hiding this comment

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

to my eyes, the line break here is weird. I'd expect the new line to occur at s.getRa(),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair.

Copy link
Member

Choose a reason for hiding this comment

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

and the indenting looks wrong.

@@ -3,6 +3,9 @@
mechanism, but we need nested lists, so we do this home-brew version
instead.
'''
from past.builtins import execfile
Copy link
Member

Choose a reason for hiding this comment

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

we should make a note to fix this in a python3 way at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix what? I would have thought all the past and builtins imports would be fixed together when we go to python3 only?

Copy link
Member

Choose a reason for hiding this comment

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

We have generally used past.builtins to give us compatibility with behavioral changes between 2 and 3. In particular using long and basestring as they are providing semantics to python 2 that do not exist on 3. execfile is not like that and it just a function that needs to be replaced with a modern approach.

@@ -102,7 +103,7 @@ def testMakeMatchStatistics(self):
"""
np.random.seed(47)
distList = list((np.random.random_sample([self.numMatches]) - 0.5) * 10)
for dist, match in itertools.izip(distList, self.matchList):
for dist, match in zip(distList, self.matchList):
Copy link
Member

Choose a reason for hiding this comment

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

from builtins import zip at top.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

# don't want this to depend on how many files e.g. py.test has open at the start.
currentOpenFiles = len(getOpenFiles())
print('NOFILE rlimit:', self.originalLimits)
resource.setrlimit(resource.RLIMIT_NOFILE, (currentOpenFiles+5, self.originalLimits[1]))
Copy link
Member

Choose a reason for hiding this comment

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

spaces need around + operator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, though my linter doesn't catch that one.

Copy link
Member

Choose a reason for hiding this comment

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

that's because we disable "spaces around operator" warnings because it's an area where there is vagueness in PEP8 and where we disagree with some interpretations.

@timj
Copy link
Member

timj commented Sep 15, 2016

Please remember to add python_future to the table file.

@parejkoj parejkoj merged commit 1c23758 into master Sep 15, 2016
@ktlim ktlim deleted the tickets/DM-7288 branch August 25, 2018 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants