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-18203: Set encoding for reading text catalogs. #158
Conversation
tests/test_readTextCatalog.py
Outdated
config = ReadTextCatalogTask.ConfigClass() | ||
config.encoding = "ascii" | ||
task = ReadTextCatalogTask(config=config) | ||
# This will generate a ResourceWarning due to a NumPy bug. |
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.
If you want to make the test output clean consider using a warnings.catch_warnings with a simplefilter to disable warning output for that command.
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.
I don't think you can, at least not trivially — I tried this, but the warning isn't generated in this code, but deep in the guts of Python: it comes from /Users/jds/Projects/LSST/stack/python/miniconda3-4.5.4/envs/lsst-scipipe-fcd27eb/lib/python3.6/traceback.py:216
sometime after this test case has finished, so a context in the test can't catch it.
I considered putting in a filter to drop all ResourceWarnings
from traceback.py:216
, but that seems less than optimal. Other suggestions welcome!
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.
Maybe it's fixed in the new numpy that we will be using from tomorrow...
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.
Pretty sure it's still broken in the latest version. :(
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.
Thanks for filing a numpy ticket on this and linking it here. However, should we have a jira ticket to remove this comment once numpy is fixed? It might be confusing in "future numpy is fixed" land...
df2264f
to
2c19f89
Compare
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.
I think utf-8 is backwards compatible with ASCII, so I'm not sure that we really need that to be configurable. Otherwise, this is fine.
encoding = pexConfig.Field( | ||
dtype=str, | ||
default="utf-8", | ||
doc="Encoding of text reference file." |
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.
Do we really need to make this configurable? Can't we just make it utf-8
forever and forget about it?
tests/test_readTextCatalog.py
Outdated
config = ReadTextCatalogTask.ConfigClass() | ||
config.encoding = "ascii" | ||
task = ReadTextCatalogTask(config=config) | ||
# This will generate a ResourceWarning due to a NumPy bug. |
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.
Thanks for filing a numpy ticket on this and linking it here. However, should we have a jira ticket to remove this comment once numpy is fixed? It might be confusing in "future numpy is fixed" land...
Your new commit is fine. Squash it. |
Commas are needed to separate fields in the header.
0f72a82
to
ea4250c
Compare
No description provided.