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.

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

BUG: fix jplspec build lookup #2945

Merged
merged 2 commits into from
Feb 5, 2024
Merged

Conversation

bsipocz
Copy link
Member

@bsipocz bsipocz commented Feb 5, 2024

This should fix the devdeps CI job.

Given the columns have a well defined name, we should just select columns based on names. Note, this hasn't caused any issues up until the 6.1dev cycle. I haven't done any bisecting to pinpoint what made it fail upstream as this fix will work for us here.

ps: no need to add a test as currently existing tests alerted us about this regression

@bsipocz bsipocz added this to the v0.4.7 milestone Feb 5, 2024
@bsipocz bsipocz linked an issue Feb 5, 2024 that may be closed by this pull request
Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

I feel like there is some unnecessary steps from Table to list/dict back to some other table, but I guess fixing that is out of scope. If this change is enough to make the code be compatible with astropy 6.1.dev, then LGTM.

Though I am curious to see if @taldcroft knows why [:] now breaks for you in astropy 6.1.dev but not in 6.0.

@bsipocz
Copy link
Member Author

bsipocz commented Feb 5, 2024

Adam has some work in progress for fixing how the lookup dictionary is used, etc, so I would not dive into anywhere beyond fixing the obvious bug.

I also don't think I have the full understanding of the desired parsing and search capabilities for these lookups and whether a dictionary subclass or a Table subclass, or not subclassing at all is the right way to do it.

Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

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

This change makes sense to me. I think it's more likely they'll change the order/number of heading parameters than it is that they'll change the NAME or TAG headings. I believe the original motivation was that I had the opposite expectation (i.e., I thought it more likely they'd change the capitalization, say, than the order) - and probably there was no reason to expect that.

@bsipocz bsipocz merged commit ab7fdaa into astropy:main Feb 5, 2024
8 of 9 checks passed
@bsipocz bsipocz deleted the BUG_jplspec_build_lookup branch April 6, 2024 03:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: JPLSpec failures with dev versions of dependencies
3 participants