Skip to content

Better migration simbad#3186

Merged
bsipocz merged 5 commits into
astropy:mainfrom
cds-astro:better-migration-simbad
Jan 22, 2025
Merged

Better migration simbad#3186
bsipocz merged 5 commits into
astropy:mainfrom
cds-astro:better-migration-simbad

Conversation

@ManonMarchand

@ManonMarchand ManonMarchand commented Jan 20, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

This PR fixes the two points raised by #3185

On the deprecated notation flux(XXX)

There was a bug when adding a list of fluxes with the deprecated API:

from astroquery.simbad import Simbad
simbad = Simbad()
simbad.add_votable_fields("flux(U)", "flux(J)")

would error instead of just warn. This was due to a nasty modification of a list in the loop that was iterating over it (introduced in #3052).

On votable fields

I figured that I based my deprecations / changes of names on the old SIMBAD API docs, and not on astroquery docs. They did not have exactly the same fields. This is the second commit of this PR. It:

  • adds support for dec_prec, diameter, otype(3), otype(s), pm, ra_prec (all from deprecated API, they now raise a warning stating the new name instead of failing)
  • fixes otype(V) and otype(S) that had a case issue (but are deprecated)
  • raises more meaningful errors on the following keywords that cannot be ported to the new API td1, sp_nature, pos, posa, typed_id, ubv, uvby1, uvby
  • add ra(d) and dec(d) as aliases for ra and dec

It would be very nice if these two changes could be back-ported to astroquery v0.4.8 as these only affect the deprecated API, and thus people migrating from 0.4.7.

@ManonMarchand ManonMarchand marked this pull request as draft January 20, 2025 16:22
@ManonMarchand ManonMarchand force-pushed the better-migration-simbad branch 2 times, most recently from 6c8c6ed to 9f6d97a Compare January 20, 2025 16:27
@pep8speaks

pep8speaks commented Jan 20, 2025 •
edited
Loading

Copy link
Copy Markdown

Hello @ManonMarchand! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2025-01-21 17:36:10 UTC

@ManonMarchand ManonMarchand force-pushed the better-migration-simbad branch from 9f6d97a to 01a8158 Compare January 20, 2025 16:28
@ManonMarchand ManonMarchand force-pushed the better-migration-simbad branch from 01a8158 to 668edf3 Compare January 20, 2025 16:32
@ManonMarchand ManonMarchand marked this pull request as ready for review January 20, 2025 16:33
@bsipocz

bsipocz commented Jan 20, 2025

Copy link
Copy Markdown
Member

Thanks Manon! I won't backport but can do a new tagged release, so people can get all the fixes we did since the release.

@ManonMarchand

ManonMarchand commented Jan 20, 2025

Copy link
Copy Markdown
Member Author

That would be nice, I hope there is nothing else remaining

@ManonMarchand

ManonMarchand commented Jan 20, 2025

Copy link
Copy Markdown
Member Author

(I also got feedback from the astronomer in charge of SIMBAD's scientific content that query_hierarchy should be detailed per default. This is way less critical... But do you know when this new tagged release would be?)

@bsipocz

bsipocz commented Jan 20, 2025

Copy link
Copy Markdown
Member

I'll try to do one towards the end of the week once a few more quick bugfixes are in, too.

@bsipocz bsipocz added this to the v0.4.9 milestone Jan 20, 2025
bsipocz
bsipocz approved these changes Jan 20, 2025
View reviewed changes

@bsipocz bsipocz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, thank you. One minor comment on the deprecation message.

Comment thread astroquery/simbad/core.py Outdated
@ManonMarchand ManonMarchand marked this pull request as draft January 21, 2025 08:59
@ManonMarchand

ManonMarchand commented Jan 21, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

Switching it to draft. I'll port all our internal tutorials from the point of view of someone who has no idea about the refactor to see if I find more pain points. This way I might catch some other deprecation messages that are unclear or buggy.
I'm doing this today.

@codecov

codecov Bot commented Jan 21, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@b005fc9). Learn more about missing BASE report.
Report is 363 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #3186   +/-   ##
=======================================
  Coverage        ?   67.42%           
=======================================
  Files           ?      229           
  Lines           ?    18612           
  Branches        ?        0           
=======================================
  Hits            ?    12550           
  Misses          ?     6062           
  Partials        ?        0

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ManonMarchand ManonMarchand force-pushed the better-migration-simbad branch from af6a90f to 59ed410 Compare January 21, 2025 16:51
@ManonMarchand ManonMarchand force-pushed the better-migration-simbad branch from 59ed410 to 9a7610d Compare January 21, 2025 17:02
@ManonMarchand

ManonMarchand commented Jan 21, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

I added ra(d) and dec(d) in the renamed arguments so that they raise a DeprecationWarning instead of an Error (squashed in the former commits)

One thing that appeared in the investigation is the new case-sensitivity of query_catalog. It's written in the docstring of the method, but I forgot to mention it in the changelog. Can I edit the 0.4.8 section of the changelog to add it? Or should it be in the 0.4.9 section even if the change happened before?

@ManonMarchand ManonMarchand marked this pull request as ready for review January 22, 2025 12:24
@bsipocz

bsipocz commented Jan 22, 2025

Copy link
Copy Markdown
Member

Thank you @ManonMarchand!

The changelog change is a tricky one, on one hand I would say don't change the 0.4.8 section, on the other people may still look into that section even when 0.4.8 is not the latest release.

However, the warning in the docs looks appropriate, we may just need that one.

@bsipocz bsipocz merged commit 1944001 into astropy:main Jan 22, 2025
ManonMarchand added a commit to cds-astro/astroquery that referenced this pull request Mar 17, 2025
@ManonMarchand
fix: remove pm from the list of fields - bug introduced in astropy#3186
0af127a
@ManonMarchand ManonMarchand mentioned this pull request Mar 17, 2025
Merged
bsipocz pushed a commit to cds-astro/astroquery that referenced this pull request Mar 17, 2025
@ManonMarchand @bsipocz
fix: remove pm from the list of fields - bug introduced in astropy#3186
f5b7dc6
bsipocz added a commit that referenced this pull request Mar 17, 2025
@bsipocz
Merge pull request #3259 from cds-astro/fix-remove-pm-field
006c62b 
[SIMBAD] fix: remove pm from the list of fields - bug introduced in #3186
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@keflavich keflavich keflavich approved these changes
@bsipocz bsipocz bsipocz approved these changes

Assignees

No one assigned

Labels

simbad

Projects

None yet

Milestone

v0.4.9

Development

Successfully merging this pull request may close these issues.

4 participants

@ManonMarchand @pep8speaks @bsipocz @keflavich