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-23281: Include GRATING in filter definition #165

Merged
merged 7 commits into from Feb 7, 2020
Merged

Conversation

timj
Copy link
Member

@timj timj commented Feb 3, 2020

No description provided.

@timj timj force-pushed the tickets/DM-23281 branch 2 times, most recently from be8d218 to 8c812eb Compare February 4, 2020 18:40
Copy link
Contributor

@mfisherlevine mfisherlevine left a comment

Choose a reason for hiding this comment

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

Comments here too, but the biggest things is the OOB communication saying that tests are failing and that filter defs need redefining (and rebasing, of course)

python/lsst/obs/lsst/translators/latiss.py Outdated Show resolved Hide resolved
python/lsst/obs/lsst/translators/latiss.py Show resolved Hide resolved
python/lsst/obs/lsst/translators/latiss.py Outdated Show resolved Hide resolved
python/lsst/obs/lsst/translators/latiss.py Outdated Show resolved Hide resolved
python/lsst/obs/lsst/translators/lsst.py Outdated Show resolved Hide resolved
@timj
Copy link
Member Author

timj commented Feb 6, 2020

You refer to test failures but those are not reproducible for me (or Jenkins) and so I don't really understand them.

@timj
Copy link
Member Author

timj commented Feb 6, 2020

I take that back. It's now failing the test for me! No idea what changed there but at least I can fix it.

@timj timj force-pushed the tickets/DM-23281 branch 5 times, most recently from 67cb97f to 1ac5862 Compare February 6, 2020 17:50
@@ -73,4 +76,31 @@
lambdaEff=0.0),
FilterDefinition(physical_filter="EMPTY",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason that EMPTY isn't just an alias for NONE and OPEN. They're physically the same, right? (I realise this is kind of not in scope as this was pre-existing, but given we're redefining some meanings here/making new categories, it's also somewhat relevant). If there's a reason I've not thought of to keep them separate just ignore me though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure where we're going long term with the AuxTel filters, but in the current configuration one filter slot has a piece of plain glass with no filtering capability, and one slot is completely empty. So we need a way to differentiate those two.

Copy link
Contributor

Choose a reason for hiding this comment

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

Plain glass is considered to be none of EMPTY, NONE or OPEN. They're listed (so far) as blank_bk7_wg05 or similar, with the string after blank_ specifying the glass type so that they can be differentiated if we choose to, and are, at present, treated as normal filters for the sake of both data management and processing. I think that's the right thing to do for all eventualities, but I'm open to persuasion, as always!

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 can't use filter aliases because butler registries don't use the aliases (only afw uses them when you ask for a filter by the "wrong" name).

@mfisherlevine
Copy link
Contributor

See what you think about the one new comment I had. Other than that, it's working great, so just rebase and merge whenever you like.

@mfisherlevine
Copy link
Contributor

(except I just tried rebasing and running the tests again, and now get one test failure, so the above is obviously modulo that 😕)

@timj
Copy link
Member Author

timj commented Feb 6, 2020

Yes, will fix. I merged some other tickets that added new test files that will need to have the grating added.

@timj timj merged commit 3a1ca26 into master Feb 7, 2020
@timj timj deleted the tickets/DM-23281 branch February 7, 2020 00:42
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

4 participants