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-35404: Add tests for focus analyses #18

Merged
merged 3 commits into from Feb 17, 2023
Merged

Conversation

mfisherlevine
Copy link
Contributor

No description provided.

Copy link

@isullivan isullivan left a comment

Choose a reason for hiding this comment

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

Looks good. To an outsider, the specific value ranges in the tests are not obvious, and a brief explanation would be helpful.

tests/test_focusAnalysis.py Outdated Show resolved Hide resolved
import unittest
from typing import Iterable

import lsst.utils.tests

Choose a reason for hiding this comment

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

Minor point, but shouldn't this import happen after lsst.summit.utils?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this referring to fact I'm importing the tested code after the mpl.use('Agg') line? This is because you have to set the matplotlib backend before any other code which imports matplotlib does (hence the #noqa: E402 line decorators) or people have plots pop up.

Choose a reason for hiding this comment

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

No, this is a very minor point. Shouldn't the order of imports be:

...
lsst.summit.utils
lsst.utils.tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohhhh, there's no isort over here, and I've made minimal effort anywhere to keep things alphabetical, so that didn't even occur to me! Look elsewhere in the package and you'll see the imports are a free-for-all! I'm considering going pre-commit + black + isort etc at some point though, but that's definitely another ticket 🙂

tests/test_focusAnalysis.py Outdated Show resolved Hide resolved
@mfisherlevine mfisherlevine merged commit 312ced8 into main Feb 17, 2023
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