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.

Sign up for GitHub

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

Jump to bottom

TST: Also run slow test in non-dev env, fix Catalog Search bug #3101

Merged
merged 4 commits into from
Jul 24, 2024
Merged

TST: Also run slow test in non-dev env, fix Catalog Search bug #3101

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4d5e291
TST: Also run slow test in non-dev env
pllim Jul 18, 2024
142dba9
BUG: Fix table I/O in Catalog Search
pllim Jul 18, 2024
1fd35ed
ID as string always
pllim Jul 23, 2024
87239a8
Force ID to be always str even from file
pllim Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci_workflows.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ jobs:
os: ubuntu-latest
python: '3.11'
toxenv: py311-test-alldeps-cov
toxposargs: --remote-data
toxposargs: --remote-data --run-slow
allow_failure: false

- name: OS X - Python 3.12
Expand Down
2 changes: 1 addition & 1 deletion CHANGES.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ Cubeviz
Imviz
^^^^^

- Added a table with catalog search results. [#2915]
- Added a table with catalog search results. [#2915, #3101]

Mosviz
^^^^^^
Expand Down
7 changes: 6 additions & 1 deletion docs/imviz/plugins.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,12 @@ catalog dropdown menu.
and works best when you only have a single image loaded in a viewer.

To load a catalog from a supported `JWST ECSV catalog file <https://jwst-pipeline.readthedocs.io/en/latest/jwst/source_catalog/main.html#output-products>`_, choose "From File...".
The file must be able to be parsed by `astropy.table.Table.read` and contain a column labeled 'sky_centroid'.
The file must be able to be parsed by `astropy.table.Table.read` and contains the following columns:

* ``'sky_centroid'``: Column with `~astropy.coordinates.SkyCoord` sky coordinates of the sources.
* ``'label'``: Column with string identifiers of the sources. If you have numerical identifiers,
you have to recast them as string first.
Copy link
Member

Choose a reason for hiding this comment

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

label is optional now, right? And won't the logic here cast a column of integers to a string, or no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In practice, I don't think a catalog is useful without identifiers. We should encourage users to pass them in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The existing "From File" logic is pretty bare-bones. All I see is:

                row_info = {'Right Ascension (degrees)': row['sky_centroid'].ra.deg,
                            'Declination (degrees)': row['sky_centroid'].dec.deg,
                            'Object ID': row.get('label', 'N/A')}

So, no, it does not turn input int column into string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I added a commit to fix that as well. Please re-re-review. Thanks!


Clicking :guilabel:`SEARCH` will show markers for any entry within the filtered zoom window.

If you have multiple viewers open, you will see another dropdown menu to select the active
Expand Down
10 changes: 4 additions & 6 deletions jdaviz/configs/imviz/plugins/catalogs/catalogs.py
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

For 'Object ID': row.get('label', -1)}, I think it does make more sense to append a number rather than an empty string since the object id is a number. Is there a significance to it being -1, or is that just a placeholder?

Copy link
Member

Choose a reason for hiding this comment

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

It was trying to append rows with incompatible data types.

@pllim - does this mean that the test case had some entries where the Object ID was populated (as an integer) and so then additional rows had to be castable to that same type? Is Object ID always going to be integer by default, couldn't it be a string for some catalogs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have definite answers for all the questions. I only know from the SDSS catalog queried in test. In that case, ID is always integer, so appending empty string crashes it. Also there is no reason to append dummy value in the first place because you should really write out the ID numbers as well to be read back in; otherwise the catalog is useless because you cannot tie the dots to anything meaningful anymore. Does that make sense or you have more concerns?

Copy link
Member

Choose a reason for hiding this comment

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

If the currently implemented catalogs always have Object ID as an integer, then I think this makes sense for now. Do you know if that's the case @kcarver1?

Copy link
Member

Choose a reason for hiding this comment

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

This will also restrict us from appending catalogs from file in which the Object ID is not an integer, is the current behavior to clear the catalog when loading from a file (after doing an online query)? If not, maybe we should change to that for now unless we're requested to support appending, in which case we'll need to figure out a way to handle the type mismatches - perhaps by always casting to a string instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, if you want to keep it as string, then more code changes are needed to always cast ID to string, so in SDSS's case, the query returns ID as int, but then when user writes it out, it will always be string. Do you want that instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the user doc might also need updating because the code looks for "label" but it is not mentioned at all in the user doc. But I will wait for your reply before I make more changes. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Handled in the latest commit.

Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ def search(self, error_on_fail=False):
for row in self.app._catalog_source_table:
row_info = {'Right Ascension (degrees)': row['ra'],
'Declination (degrees)': row['dec'],
'Object ID': row['objid']}
'Object ID': row['objid'].astype(str)}
self.table.add_item(row_info)

elif self.catalog_selected == 'From File...':
Expand All @@ -175,11 +175,9 @@ def search(self, error_on_fail=False):
skycoord_table = table['sky_centroid']

for row in self.app._catalog_source_table:
# find new to add in a way to append the source id to the table
# 'Object ID': row['label']} ; 'label' is failing tests
row_info = {'Right Ascension (degrees)': row['sky_centroid'].ra,
'Declination (degrees)': row['sky_centroid'].dec,
'Object ID': row.get('label', '')}
row_info = {'Right Ascension (degrees)': row['sky_centroid'].ra.deg,
'Declination (degrees)': row['sky_centroid'].dec.deg,
'Object ID': row.get('label', 'N/A')}
self.table.add_item(row_info)

else:
Expand Down
6 changes: 3 additions & 3 deletions jdaviz/configs/imviz/tests/test_catalogs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,15 +105,15 @@ def test_plugin_image_with_result(self, imviz_helper, tmp_path):

# test loading from file
table = imviz_helper.app._catalog_source_table
skycoord_table = SkyCoord(table['ra'], table['dec'], unit='deg')
qtable = QTable({'sky_centroid': skycoord_table})
qtable = QTable({'sky_centroid': SkyCoord(table['ra'], table['dec'], unit='deg'),
'label': table['objid'].astype(str)})
tmp_file = tmp_path / 'test.ecsv'
qtable.write(tmp_file, overwrite=True)

catalogs_plugin.from_file = str(tmp_file)
# setting filename from API will automatically set catalog to 'From File...'
assert catalogs_plugin.catalog.selected == 'From File...'
catalogs_plugin.vue_do_search()
catalogs_plugin.search(error_on_fail=True)
assert catalogs_plugin.results_available
assert catalogs_plugin.number_of_results == prev_results

Expand Down