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

Better migration simbad #3186

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ManonMarchand
Copy link
Member

@ManonMarchand ManonMarchand commented Jan 20, 2025

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
Copy link

pep8speaks commented Jan 20, 2025

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
Copy link
Member

bsipocz commented Jan 20, 2025

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
Copy link
Member Author

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

@ManonMarchand
Copy link
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
Copy link
Member

bsipocz commented Jan 20, 2025

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
Copy link
Member

@bsipocz bsipocz left a comment

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.

astroquery/simbad/core.py Outdated Show resolved Hide resolved
@ManonMarchand ManonMarchand marked this pull request as draft January 21, 2025 08:59
@ManonMarchand
Copy link
Member Author

ManonMarchand commented Jan 21, 2025

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.

Copy link

codecov bot commented Jan 21, 2025

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 1 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.

@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
Copy link
Member Author

ManonMarchand commented Jan 21, 2025

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants