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.

Sign up for GitHub

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

Jump to bottom

Vectorize HEASARC object queries #3013

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

Vectorize HEASARC object queries #3013

wants to merge 4 commits into from

Conversation

nkphysics
Copy link
Member

Howdy all, been a while since I last contributed.

I've been sitting on this PR for a little while and figured its better late than never to contribute it. It refactors Heasarc.query_object_async()'s object arg to accept iterables of multiple objects (see example below). I think its a step in the right direction of what #682 is all about, but looking at recent PRs it might conflict with #2997.

The changes here are small and something similar could certainly be applied to region queries and maybe the mission fields too, but more discussion might be needed for the mission fields since the table columns don't always match which could make responses messy.

Idk if the changes in this PR is something we're still interested in, but if so here's a small start.

>>> from astroquery.heasarc import Heasarc
>>> heasarc = Heasarc()
>>> table = heasarc.query_object(object_name=["PSR B0531+21", "SAX J1808.4-3658"], mission='nicermastr')
>>> table.pprint()
                       NAME                            RA       DEC          TIME       ... PUBLIC_DATE  OBS_TYPE        SEARCH_OFFSET_      
                                                      deg       deg          mjd        ...     mjd                                          
-------------------------------------------------- --------- --------- ---------------- ... ----------- ---------- --------------------------
PSR_B0531+21                                        83.63326  22.01412     58478.021562 ...       58492 CAL            0.023 (PSR B0531+21)\n
PSR_B0531+21                                        83.63326  22.01426     58190.122500 ...       58238 CAL            0.016 (PSR B0531+21)\n
PSR_B0531+21                                        83.63336  22.01418     58191.016620 ...       58238 CAL            0.023 (PSR B0531+21)\n
PSR_B0531+21                                        83.63340  22.01430     58481.366435 ...       58495 CAL            0.019 (PSR B0531+21)\n
PSR_B0531+21                                        83.63346  22.01399     58479.372720 ...       58493 CAL            0.035 (PSR B0531+21)\n
PSR_B0531+21                                        83.63300  22.01445     58451.429572 ...       58465 CAL            0.007 (PSR B0531+21)\n
PSR_B0531+21                                        83.63449  22.01708     58793.987731 ...       58808 CAL            0.173 (PSR B0531+21)\n
PSR_B0531+21                                        83.63280  22.01421     58532.994965 ...       58547 CAL            0.024 (PSR B0531+21)\n
PSR_B0531+21                                        83.63336  22.01547     58430.516377 ...       58444 CAL            0.061 (PSR B0531+21)\n
PSR_B0531+21                                        83.63467  22.01693     58431.480833 ...       58445 CAL            0.170 (PSR B0531+21)\n
PSR_B0531+21                                        83.63381  22.01551     58443.453704 ...       58457 CAL            0.073 (PSR B0531+21)\n
PSR_B0531+21                                        83.63337  22.01420     58480.359259 ...       58494 CAL            0.022 (PSR B0531+21)\n
PSR_B0531+21                                        83.63347  22.01416     58121.382106 ...       58181 CAL            0.028 (PSR B0531+21)\n
                                               ...       ...       ...              ... ...         ...        ...                        ...
SAX_J1808.4-3658                                   272.11090 -36.97686     59840.070995 ...       59884 NOR        0.224 (SAX J1808.4-3658)\n
SAX_J1808.4-3658                                   272.11850 -36.98186     59821.493704 ...       59835 NOR        0.250 (SAX J1808.4-3658)\n
SAX_J1808.4-3658                                   272.11540 -36.97887     58781.312882 ...       58795 NOR        0.032 (SAX J1808.4-3658)\n
SAX_J1808.4-3658                                   272.11550 -36.97923     58730.995347 ...       58745 NOR        0.039 (SAX J1808.4-3658)\n
SAX_J1808.4-3658                                   272.11570 -36.97854     58780.409884 ...       58794 NOR        0.052 (SAX J1808.4-3658)\n
SAX_J1808.4-3658                                   272.11520 -36.97866     58784.537211 ...       58798 NOR        0.028 (SAX J1808.4-3658)\n
SAX_J1808.4-3658                                   272.11640 -36.97831     58779.377697 ...       58793 NOR        0.088 (SAX J1808.4-3658)\n
SAX_J1808.4-3658                                   272.11500 -36.97884     58789.005787 ...       58803 NOR        0.014 (SAX J1808.4-3658)\n
SAX_J1808.4-3658                                   272.11570 -36.97902     59872.225231 ...       59916 NOR        0.046 (SAX J1808.4-3658)\n
SAX_J1808.4-3658                                   272.11700 -36.97735     58778.540370 ...       58792 NOR        0.145 (SAX J1808.4-3658)\n
SAX_J1808.4-3658                                   272.11500 -36.97894     58786.279039 ...       58800 NOR        0.012 (SAX J1808.4-3658)\n
SAX_J1808.4-3658                                   272.11520 -36.97892     58795.179977 ...       58809 NOR        0.022 (SAX J1808.4-3658)\n
SAX_J1808.4-3658                                   272.11560 -36.97887     58782.022361 ...       58796 NOR        0.041 (SAX J1808.4-3658)\n
SAX_J1808.4-3658                                   272.11520 -36.97919     59875.919213 ...       59919 NOR        0.025 (SAX J1808.4-3658)\n
Length = 281 rows

@pep8speaks
Copy link

pep8speaks commented May 27, 2024 •
edited
Loading

Hello @nkphysics! 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 2024-05-27 21:40:54 UTC

keflavich
keflavich approved these changes May 27, 2024
View reviewed changes
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.

lgtm. This is a very simple improvement, and it's nice that HEASARC supports this.

astroquery/heasarc/core.py
object_name : str
Object to query around. To set search radius use the 'radius'
object_name : str or iterable
Objects to query around. To set search radius use the 'radius'
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you specify what the iterable is here? i.e., the iterable must be an iterable (list, tuple, ...) of strings

Copy link
Member Author

Choose a reason for hiding this comment

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

Can do. I tested with lists and tuples, but should work with any iterable

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the wording here to object_name : str or iterable (list or tuple) of strings

nkphysics
nkphysics commented May 27, 2024
View reviewed changes
astroquery/heasarc/tests/test_heasarc_remote_isdc.py
# Assert that there are indeed numerous tables from ISDC
# Qunatity of tables could change due to breakdown of the descriptions
# Accessing via isdc returns only Integral related tables from HEASARC
assert len(missions) >= 5
Copy link
Member Author

Choose a reason for hiding this comment

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

I made this change to test_mission_list since it wasn't passing and the previous comment was inaccurate. In short, accessing heasarc from isdc will just return stuff related to the integral mission. The 5 tables that were expected before have now grown to 8 since some have been added with proposal and description info.

I just followed the lead of the heasarc remote tests and just asserted that as long as we are getting multiple tables (from what was previously seen), that should be ok.

@nkphysics
Copy link
Member Author

  • Changelog entry added
  • New data added for local tests
  • Rewording completed as requested

Lmk if there's anything else yall are looking for with this. Thanks! 😄

bsipocz
bsipocz requested changes Jun 10, 2024
View reviewed changes
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.

I would push back on this as suggest closing the PR without merging, and maybe revisit the issue once the complete rewrite of the module have been merged. For more see #2997

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsipocz bsipocz bsipocz requested changes

@keflavich keflavich keflavich approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nkphysics @pep8speaks @keflavich @bsipocz