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

DM-7258: Port to Python 3 #5

Merged
merged 12 commits into from Aug 20, 2016
Merged

DM-7258: Port to Python 3 #5

merged 12 commits into from Aug 20, 2016

Conversation

timj
Copy link
Member

@timj timj commented Aug 19, 2016

No description provided.

I preserved one table in a test that used extra whitespace
to improve readability
Instead of testing two separate arrays, the same array
was being compared to itself (because the two variables
in question were views to the same underlying data).
Also use assertMasksEqual instead of custom code.
Remove some unused imports
Standardize import order
Fix a few lines that were too long
I left one flake8 warning about whitespace for clarity

pexLog.Trace_setVerbosity("lsst.coadd.utils", Verbosity)

try:
dataDir = lsst.utils.getPackageDir('afwdata')
medMIPath = os.path.join(dataDir, "data", "med.fits")
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll make the general comment that the tests use medMIPath but skip on dataDir. The tests themselves don't use dataDir so I think I'd prefer it if medMIPath was the thing that was set to None and was the thing that the skip test used. It seems more consistent to me to skip on the thing that the test is using.

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the code as written because the skip is caused by afwdata not being setup. I think it is clearer to test on that, so that the test message matches the thing being tested, and it is more forward looking in that we may decide to add tests that use additional data from afwData. That said, the I strongly prefer the naming in coadd.py, e.g. AfwDataDir, as and a capitalized name for the file being used.

An alternative you may prefer is for each test to generate the path to the file being tested itself, so the only global is AfwDataDir. For coadd.py I'm not very happy with that only because the same file is used in 3 places, but I guess I could make the subpath a module global and repeat the os.path.join in each test.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have committed that alternative, changing both tests that you commented on. I hope you like it.

Use AfwdataDir as the path to the afwData package.
Add another constant for the path to the desired data file(s),
relative to AfwdataDir (instead of absolute and only defined
if afwdata is setup).
Build the path to the final desired image(s) using os.path.join
when needed.
@r-owen r-owen merged commit 295d23f into master Aug 20, 2016
@ktlim ktlim deleted the tickets/DM-7258 branch August 25, 2018 05:02
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