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-25692: Add gen3 formatter for Filter #268

Merged
merged 1 commit into from Jun 26, 2020
Merged

DM-25692: Add gen3 formatter for Filter #268

merged 1 commit into from Jun 26, 2020

Conversation

timj
Copy link
Member

@timj timj commented Jun 26, 2020

This allows an Exposure to be disassembled and assembled without
relying on adding something to metadata.

Copy link
Member

@kfindeisen kfindeisen left a comment

Choose a reason for hiding this comment

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

There are some issues with the type handling in the new code (there's a mix of generic and highly type-specific references to the filter), but nothing that should require re-review.

python/lsst/obs/base/formatters/filter.py Outdated Show resolved Hide resolved
python/lsst/obs/base/formatters/filter.py Outdated Show resolved Hide resolved
python/lsst/obs/base/formatters/filter.py Outdated Show resolved Hide resolved
python/lsst/obs/base/formatters/filter.py Show resolved Hide resolved
python/lsst/obs/base/formatters/filter.py Outdated Show resolved Hide resolved

Raises
------
Exception
Copy link
Member

Choose a reason for hiding this comment

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

Please be more specific. We shouldn't be encouraging people to catch Exception.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the general sense of any formatter, we have no real idea what exceptions could be raised. It doesn't really matter either because all formatter exceptions are caught by datastore.


Parameters
----------
inMemoryDataset : `object`
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit contradictory, and I'm not sure what to do about it. On the one hand, formal type safety requires that this be an object of any type. On the other hand, the method body explicitly assumes that inMemoryDataset is a Filter, and will raise an AttributeError (convert to TypeError?) for any other type.

Copy link
Member Author

Choose a reason for hiding this comment

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

We haven't yet developed a convention for someone configuring their datastore such that the wrong python type turns up in the formatter.

python/lsst/obs/base/formatters/filter.py Outdated Show resolved Hide resolved
@timj
Copy link
Member Author

timj commented Jun 26, 2020

Thanks. Sorry about all the docstring problems. I copied them from YamlFormatter and forgot to update them.

This allows an Exposure to be disassembled and assembled without
relying on adding something to metadata.
@timj timj merged commit 1eddb07 into master Jun 26, 2020
@timj timj deleted the tickets/DM-25692 branch June 26, 2020 23:33
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