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

[pllim]

Probably an oversight to only run slow tests on devdeps. I bet it would also fail on stable astropy but let's see. Yes, it did. I bet the code never worked in the first place. It was trying to append rows with incompatible data types.

🐱

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.92%. Comparing base (143cd8d) to head (87239a8).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3101      +/-   ##
==========================================
+ Coverage   88.78%   88.92%   +0.13%     
==========================================
  Files         111      111              
  Lines       17372    17372              
==========================================
+ Hits        15424    15448      +24     
+ Misses       1948     1924      -24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kcarver1]

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?

[kecnry]

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?

[pllim]

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?

[kecnry]

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?

[kecnry]

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.

[pllim]

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?

[pllim]

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!

[pllim]

Handled in the latest commit.

[haticekaratay]

Approving with the assumption that ObjectID is an integer. This does the fix! Thanks @pllim

[pllim]

@haticekaratay , ID is now always string instead of integer. Do you want to re-review? Thanks!

[kecnry]

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

[pllim]

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

[pllim]

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.

[pllim]

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

[kecnry]

Thanks!