Codecov Report

Merging #8745 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #8745      +/-   ##
==========================================
- Coverage   86.96%   86.94%   -0.02%     
==========================================
  Files         408      399       -9     
  Lines       61437    59355    -2082     
  Branches     1096     1100       +4     
==========================================
- Hits        53429    51607    -1822     
+ Misses       7385     7107     -278     
- Partials      623      641      +18

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b96c6f3...2196ad0. Read the comment docs.

Until I hear back from @tomdonaldson , this is not going forward, so removing milestone for now.

but surely having a decision by 4.0 is feasible?

I have locally rebased this on master and changed the Cython code update to below, and see tests passing for Table and io.votable:

    try:
        if value.dtype.char == 'S' and not value.shape:
            value = value.decode('utf-8', errors='replace')
    except AttributeError:
        if isinstance(value, bytes):
            value = value.decode('utf-8', errors='replace')
    return value

With this new flow, I don't see any performance change for Column item access. That is expected because it never gets to the if isinstance() code in this case (no attribute error since value will be a numpy object with dtype and shape attributes).

I confirmed that there is an API change for table access to an Object column that has a Python bytes object inside. In release it returns bytes, with this it will return str. At some level that is more consistent with Table behavior, but in any case that seems like a relatively rare situation.

Takeaway: this is good from my perspective for Table, but would definitely need an API changelog entry.

Re: #8745 (comment)

@taldcroft , regarding the code change that you posted above -- Do you want me to actually change it to that logic or was it just for your personal testing?

I am not sure what you mean by "it never gets to the if isinstance() code in this case" because the reason I added it was to handle the VO use case, so I would think the logic flow would actually get there, since you said you noticed a change in behavior. I'd rather not have the check in except block because if I remember correctly, except is computationally "expensive" and VO already has its own performance issues without adding more to it.

Hope you can clarify!

p.s. I updated the change log and will push it out shortly.

@taldcroft , regarding the code change that you posted above -- Do you want me to actually change it to that logic or was it just for your personal testing?

I did want you to change it to maintain table performance. Yesterday I thought I found a noticeable hit on column access speed with the `isinstance() in there up front. But trying to reproduce today I don't see it. E.g.:

col = Column([1, 2, 3])
timeit col[1]

or

col = Column(np.arange(1e6))
timeit l = [c for c in col]

Can you verify that the timing in this branch compared to master is the same? I'm surprised / confused because it has to do the isinstance first and then do the original test as well. In my testing isinstance() takes about 100 ns compared to the column access time of 300 ns in master. So I would have expected to get about 100 ns more in this branch vs. master, but it came out just the same. Don't forget the Cython needs to be recompiled in different versions.

If there is no noticeable performance difference then I agree avoiding the except: is a good thing.

I am not sure what you mean by "it never gets to the if isinstance() code in this case"

Sorry, I just meant for normal Table column access of some type like float or int.

This change has a side effect on write() that FIELDs with string values now default to an IVOA datatype of "unicodeChar" instead of "char". This change was noted and accounted for in the unit tests, but it turns out it is not benign.

The output() method of the UnicodeChar converter is different than that for Char. One impact of this is that when a VOTable with a char FIELD is read, then written, the FIELD is output as a unicodeChar with it original value wrapped by b''. So when input is:
...
<FIELD ID="mydata" arraysize="*" datatype="char" name="mydata"/>
<DATA>
<TABLEDATA><TR><TD>My Data Value</TD></TR></TABLEDATA>
...

the output is:

...
<FIELD ID="mydata" arraysize="*" datatype="unicodeChar" name="mydata"/>
<DATA>
<TABLEDATA><TR><TD>b'My Data Value'</TD></TR></TABLEDATA>
...

With this complication, and the lingering concerns about performance and Table impacts beyond VOTable reading, @pllim and I decided we should table this for 4.0 and revisit it for 4.1.

Trying to see if we can do this without touching the Table code and end up deep in 🐰 🕳

Closing in favor of #9505 . If we want to come back to this solution, I'd rather open a new PR.