API/BUG: only output user-requested columns in votable.parse by neutrinoceros · Pull Request #15959 · astropy/astropy

neutrinoceros · 2024-01-27T18:18:11Z

Description

Fixes #14943
I took me a couple hours to pass my test and all other tests locally, so I'm opening the PR while iron is hot, but I expect I'll probably want to refactor a bit before I open to review.

Since I needed to adjust existing tests which were actively checking for previous behaviour, I'm marking this as an API change, but in my opinion it's borderline a bug fix. In other words, I'm aiming astropy 6.1.0, not 6.0.1.

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2024-01-27T18:18:58Z

github-actions · 2024-01-27T18:19:03Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

neutrinoceros · 2024-01-28T11:20:46Z

I'm not convinced that failures are related. In anywise I don't reproduce them locally. Might have something to do with pytest 8.0.0...

eerovaher

I am not doing a thorough review, I'm just commenting on a few things that stood out to me.

eerovaher · 2024-01-29T18:49:24Z

        # Deal with a subset of the columns, if requested.
        if not columns:
-            colnumbers = list(range(len(fields)))
+            colnumbers = list(range(len(all_fields)))


Why is the conversion from range to list necessary?

because range() produces a 1-time consumable iterator, but this variable is definitely used more than once.

There is no limit to how many times a range instance can be iterated over:

>>> one_to_three = range(1, 4) >>> for i in one_to_three: ... for j in one_to_three: ... print(i, j) ... 1 1 1 2 1 3 2 1 2 2 2 3 3 1 3 2 3 3

Oh I didn't know that. Was it always true ?

As far as I know this was true even for xrange in Python 2 (but I can't claim to know Python 2 very well).

I am willing to guess that quite a few people might believe a range to be single-use because they know that range does lazy evaluation and from that they conclude (mistakenly) that range must be a generator.

that would make sense, I probably fell in that trap too.

eerovaher · 2024-01-29T18:54:28Z

+Only output user-requested columns in ``votable.parse``.
+Previously, unselected columns would just be masked (and unallocated).


The recommended way to read VOTables is through the unified I/O interface, so I'd expect the change log entry to describe how that is affected.

neutrinoceros · 2024-01-30T09:52:50Z

@eerovaher This is far from redu for review (I actually left it in the middle of a refactor). I'll answer your questions tomorrow when I'm back at my desk !

neutrinoceros · 2024-02-01T09:40:30Z

It took me a while to complete, but I'm almost done with refactoring my initial wonky patch. After I'm sure that the refactor still survives CI, I'll squash my history. Next thing on the list is to make sure that TableElement._parse_binary also works correctly when colnumbers is not None.

neutrinoceros · 2024-02-01T15:19:21Z

Okay I think this should now be ready for review.

neutrinoceros · 2024-02-22T13:33:55Z

@eerovaher, do you have any other suggestions or requests ? also pinging @pllim as a package expert !

pllim · 2024-02-22T14:35:24Z

This would affect PyVO world the most, so I am asking a review from Tom D and maybe ping @lmichel too as a stakeholder.

lmichel · 2024-02-22T17:04:28Z

The MIVOT annotation readout in Astropy does not care about the table columns at all. It is a simple extension to the resource parsing that extracts a piece of XML.
The connection between the MIVOT annotations and the data rows (table readout) is done in PyVO (PR #497).
Currently our PyVO implementation does not do any column filtering (although it should).
This fix you are proposing shouldn't affect my PyVO code, and if it does, I'll fix it as the PR is not merged.

Co-authored-by: P. L. Lim <[email protected]>

neutrinoceros · 2024-04-18T14:38:10Z

rebased !

tomdonaldson · 2024-04-18T16:02:00Z

-        fields = self.fields
+        for i, field in enumerate(self.all_fields):
+            if i in colnumbers and field not in self.fields:
+                self.fields.append(field)


If this method is called repeatedly with different colnumbers, self.fields could end up out of order and with fields not in colnumbers. Also, for future protection if we lock down public access to self.fields, the append should probably be on self._fields (e.g., if later we do something like replace .append() with .add_field()).

If we do anything to fix this now, it should probably be to totally rebuild self.fields from colnumbers, but I'm uncomfortable with these side effects. Ideally creating arrays shouldn't modify any attributes of the table but also ideally the encapsulation (public vs. private) aspects of constructing a table could be cleaned up quite a bit, so I'm hesitant to do anything radical in this PR.

good point. Reconstructing self._fields entirely seems safer and isn't really more complicated. How about this ?

diff --git a/astropy/io/votable/tree.py b/astropy/io/votable/tree.py index 08dade28db..5df03cdf0e 100644 --- a/astropy/io/votable/tree.py +++ b/astropy/io/votable/tree.py @@ -2607,9 +2607,12 @@ class TableElement( if colnumbers is None: colnumbers = list(range(len(self.all_fields))) - for i, field in enumerate(self.all_fields): - if i in colnumbers and field not in self.fields: - self.fields.append(field) + selected_fields = HomogeneousList( + Field, + values=[f for i, f in enumerate(self.all_fields) if i in colnumbers], + ) + if self._fields != selected_fields: + self._fields = selected_fields fields = self.all_fields

We could also add a warning if reconstruction is triggered.

That implementation looks reasonable to me. It's interesting (and fine) that references to all_fields ensure that all_fields is caught up with anything in fields just prior to syncing in the other direction. (I'm not a huge fan of properties since they make such side-effects non-obvious, but that's my own hangup.)

I can see the motivation for a warning in that it would be an usual and possibly unexpected circumstance, but I also wonder if we could come up with succinct wording that a user could understand and act on. Unless the reconstruction happens due to a logic error we haven't noticed, the user would be doing some fairly unusual things to trigger this (as we'll see if you add a test case for this), so I think a warning is probably not worth the effort.

I pushed it as is. Thank you so much for your thoughtful review !

Thank you for doing the work and being so responsive to questions and comments! Could you go ahead and create an issue for the proposed follow-on deprecation patch?

It's getting late, but this will be top of my task list for tomorrow !

No worries. I opened #16314 for you. Feel free to add details there tomorrow as you see fit. Thanks!

tomdonaldson

After the last flurry of activity, I think this is ready again. Thanks everyone for being so thorough!

pllim · 2024-04-18T21:32:10Z

Thanks, all!

* TST: add a regression test for bug 14943 * TST: adjust existing tests * API: only output user-requested columns in votable.parse * raise an error when encountering a 'FITS' table while requesting specific columns (unsupported case) * TST: add a test case for parsing VOTable as binary with selected columns * BUG: fix support for parting VOTable as binary with selected columns * RFC: remove 'fields' argument from TableElement._parse_* private methods (use the internal state instead) * TST: replace repetition with parametrization * remove unnecessary type annotation * CLN: cleanup unneeded __future__ import * RFC: fix a broken reference name * fix typo * keep all_fields in sync with fields attribute as much as possible * consistent use of del Co-authored-by: P. L. Lim <[email protected]> * improve comment * update docstring and changelog * a more robust impl of TableElement.create_arrays --------- Co-authored-by: P. L. Lim <[email protected]>

msdemlei

Disclaimer: I don't know enough about astropy's VOTable implementation to confidently say I could spot subtle issues, and I certainly would not have thought about the problem with the column order in all_fields. Having said that, I agree with this PR's intent, and the implementation is sane and reasonably clear.

I also agree that appending to fields should be deprecated. For incrementally building table contents, there's astropy.Table itself, and I don't see an overriding reason to enable this in VOTable. There are certainly good reasons to keep VOTable metadata mutable elsewhere (PARAM-s, GROUP-s, INFO-s, etc). But since mutability is a curse, we should not have it where there are good alternatives.

neutrinoceros · 2024-08-20T07:09:38Z

I also agree that appending to fields should be deprecated.

@msdemlei maybe you meant to comment directly on #16316 ?

msdemlei · 2024-08-20T07:29:04Z

On Tue, Aug 20, 2024 at 12:10:00AM -0700, Clément Robert wrote: > I also agree that appending to fields should be deprecated. @msdemlei maybe you meant to comment directly on #16316 ?

Not really; I had not bothered to check whether this one was already merged, and I was asked to review it. If there's any dissent about the deprecation, I'll be happy to repeat my endorsement there...

pllim · 2024-08-20T13:04:50Z

Thanks for the comments. Would help for future reference regardless.

github-actions Bot added the io.votable label Jan 27, 2024

neutrinoceros changed the title ~~BUG: only parse user-requested columns in votable.parse~~ BUG: only output user-requested columns in votable.parse Jan 27, 2024

neutrinoceros force-pushed the io/votable/bug/only_parse_requested_columns branch from 7b5722c to b1b0ee1 Compare January 27, 2024 18:22

neutrinoceros changed the title ~~BUG: only output user-requested columns in votable.parse~~ API/BUG: only output user-requested columns in votable.parse Jan 27, 2024

neutrinoceros force-pushed the io/votable/bug/only_parse_requested_columns branch from b1b0ee1 to b1e192c Compare January 28, 2024 09:56

neutrinoceros force-pushed the io/votable/bug/only_parse_requested_columns branch from b1e192c to 370c2e1 Compare January 29, 2024 10:06

eerovaher reviewed Jan 29, 2024

View reviewed changes

pllim added this to the v6.1.0 milestone Jan 29, 2024

neutrinoceros force-pushed the io/votable/bug/only_parse_requested_columns branch 4 times, most recently from 63fd1ee to cab61cf Compare February 1, 2024 09:35

neutrinoceros force-pushed the io/votable/bug/only_parse_requested_columns branch 5 times, most recently from 19cddb9 to 5a26dbd Compare February 1, 2024 15:18

neutrinoceros marked this pull request as ready for review February 1, 2024 15:19

eerovaher reviewed Feb 1, 2024

View reviewed changes

Comment thread astropy/io/votable/tests/test_vo.py Resolved

Comment thread astropy/io/votable/tree.py Resolved

neutrinoceros force-pushed the io/votable/bug/only_parse_requested_columns branch from 5a26dbd to 3bbb6d2 Compare February 1, 2024 18:00

pllim requested a review from tomdonaldson February 22, 2024 14:35

neutrinoceros and others added 8 commits April 18, 2024 16:38

TST: replace repetition with parametrization

c567853

remove unnecessary type annotation

4be8e51

CLN: cleanup unneeded __future__ import

34307fb

RFC: fix a broken reference name

2852893

fix typo

0d03c88

keep all_fields in sync with fields attribute as much as possible

9d2487f

consistent use of del

c4803ea

Co-authored-by: P. L. Lim <[email protected]>

improve comment

8865d51

neutrinoceros force-pushed the io/votable/bug/only_parse_requested_columns branch from 84a5c05 to 8865d51 Compare April 18, 2024 14:38

update docstring and changelog

e100eb3

tomdonaldson reviewed Apr 18, 2024

View reviewed changes

a more robust impl of TableElement.create_arrays

b1ee421

tomdonaldson approved these changes Apr 18, 2024

View reviewed changes

pllim added the API change PRs and issues that change an existing API, possibly requiring a deprecation period label Apr 18, 2024

pllim merged commit 4ab38bc into astropy:main Apr 18, 2024

neutrinoceros deleted the io/votable/bug/only_parse_requested_columns branch April 18, 2024 21:32

pllim mentioned this pull request Apr 18, 2024

API: Make fields property immutable #16314

Open

neutrinoceros mentioned this pull request Apr 20, 2024

DEPR: deprecate direct mutations of TableElement.fields by making it an owned list #16316

Open

3 of 5 tasks

tomdonaldson requested review from andamian and msdemlei and removed request for andamian and msdemlei August 19, 2024 22:01

msdemlei reviewed Aug 20, 2024

View reviewed changes

pllim mentioned this pull request Nov 12, 2026

Keep columns aligned when invalid="mask" masks a VOTable TABLEDATA value #20836

Merged

1 of 2 tasks

		Only output user-requested columns in ``votable.parse``.
		Previously, unselected columns would just be masked (and unallocated).

Conversation

neutrinoceros commented Jan 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions Bot commented Jan 27, 2024

Uh oh!

github-actions Bot commented Jan 27, 2024

Uh oh!

neutrinoceros commented Jan 28, 2024

Uh oh!

eerovaher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

neutrinoceros commented Jan 30, 2024

Uh oh!

neutrinoceros commented Feb 1, 2024

Uh oh!

neutrinoceros commented Feb 1, 2024

Uh oh!

Uh oh!

Uh oh!

neutrinoceros commented Feb 22, 2024

Uh oh!

pllim commented Feb 22, 2024

Uh oh!

lmichel commented Feb 22, 2024

Uh oh!

neutrinoceros commented Apr 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomdonaldson Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomdonaldson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Apr 18, 2024

Uh oh!

msdemlei left a comment

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commented Aug 20, 2024

Uh oh!

msdemlei commented Aug 20, 2024 via email

Uh oh!

pllim commented Aug 20, 2024

neutrinoceros commented Jan 27, 2024 •

edited

Loading

neutrinoceros Apr 18, 2024 •

edited

Loading

tomdonaldson Apr 18, 2024 •

edited

Loading

tomdonaldson left a comment •

edited

Loading