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
DM-22470: Remove all uses of past and future from fgcm #10
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good but two real comments:
- Do not import
builtins
at all since it is a no-op. - Please also remove
__future__
@@ -1,5 +1,6 @@ | |||
from __future__ import division, absolute_import, print_function | |||
from past.builtins import xrange | |||
|
|||
from builtins import range |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this since it's not needed in python3.
|
||
from builtins import range |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
@@ -1,6 +1,6 @@ | |||
from __future__ import division, absolute_import, print_function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove all the __future__
imports as well. Not needed.
@@ -1,5 +1,6 @@ | |||
from __future__ import division, absolute_import, print_function | |||
from past.builtins import xrange | |||
|
|||
from builtins import range |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove.
@@ -1,5 +1,5 @@ | |||
from __future__ import division, absolute_import, print_function | |||
from past.builtins import xrange | |||
from builtins import range |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove. I won't comment further on unnecessary use of from builtins import
('BANDS','a2',len(self.bands)), | ||
('FITBANDS','a2',len(self.fitBands)), | ||
('NOTFITBANDS','a2',len(self.notFitBands)), | ||
('LUTFILTERNAMES', 'a%d' % (maxFilterLen), len(self.lutFilterNames)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you but I think an f-string would look cleaner here.
@@ -191,8 +194,8 @@ def computeZeropoints(self): | |||
('FGCM_APERCORR','f8'), | |||
('FGCM_CTRANS', 'f4'), | |||
('EXPTIME','f4'), | |||
('FILTERNAME','a2'), | |||
('BAND','a2'), | |||
('FILTERNAME', 'a%d' % (maxFilterLen)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f-string?
Remove all uses of past and future, and update to v2.4.1.