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

New hyperleda service 298 #2023

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

Conversation

iskren-y-g
Copy link

Hi all,

I am not sure if there has been any interest/progress on this, but since I already worked on this for my own projects, I decided to prepare a new class to query from HyperLEDA in relation to [#298].

I have followed closely the astropy's guidelines as well as the astroquery's template module to prepare the module and simple tests to match the astroquery API. I also placed a doc example in astroquery/docs/hyperleda/

All tests passed here, so, I hope it is in a decent shape for a review.

@pep8speaks
Copy link

pep8speaks commented Mar 22, 2021

Hello @iskren-y-g! 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 2021-03-22 17:44:46 UTC

@codecov
Copy link

codecov bot commented Mar 22, 2021

Codecov Report

Merging #2023 (3ac1663) into master (c4dde9d) will increase coverage by 2.58%.
The diff coverage is 73.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2023      +/-   ##
==========================================
+ Coverage   64.45%   67.03%   +2.58%     
==========================================
  Files         200      416     +216     
  Lines       15930    27300   +11370     
==========================================
+ Hits        10267    18300    +8033     
- Misses       5663     9000    +3337     
Impacted Files Coverage Δ
astroquery/hyperleda/tests/setup_package.py 50.00% <50.00%> (ø)
...stroquery/hyperleda/tests/test_hyperleda_remote.py 50.00% <50.00%> (ø)
astroquery/hyperleda/core.py 57.69% <57.69%> (ø)
astroquery/splatalogue/load_species_table.py 89.47% <66.66%> (ø)
astroquery/hyperleda/tests/test_hyperleda.py 93.75% <93.75%> (ø)
astroquery/simbad/core.py 89.66% <95.00%> (+0.23%) ⬆️
astroquery/hyperleda/__init__.py 100.00% <100.00%> (ø)
astroquery/sdss/core.py 86.33% <100.00%> (ø)
astroquery/sdss/tests/test_sdss.py 94.57% <100.00%> (ø)
astroquery/sha/core.py 70.58% <100.00%> (ø)
... and 262 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8248ba8...3ac1663. Read the comment docs.

@keflavich
Copy link
Contributor

Thanks for the contribution!

@low-sky, since you requested this long long ago, are you interested in reviewing this PR (or at least testing it out)?

@bsipocz bsipocz added this to the v0.4.3 milestone Apr 20, 2021
@bsipocz bsipocz modified the milestones: v0.4.3, v0.4.4 Jul 7, 2021
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 is a great contribution, thanks! And, sorry there was a bit of delay in reviewing - I'm always backlogged.

I've left several style and cleanup requests. I believe this is actually in working order and can be merged after a bit of cleanup though. Nice work!


# Based on: https://astroquery.readthedocs.io/en/latest/template.html

# Imports organized as shown below
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 remove these various comments that are part of the template/tutorial?


param_str = str(param_lst)
param_str = param_str[2:param_str.rfind('\'')]
param_str = param_str.replace('\', \'', ',')
Copy link
Contributor

Choose a reason for hiding this comment

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

this might be more readable as:

Suggested change
param_str = param_str.replace('\', \'', ',')
param_str = param_str.replace("', '", ',')

param_lst.remove(param)

param_str = str(param_lst)
param_str = param_str[2:param_str.rfind('\'')]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
param_str = param_str[2:param_str.rfind('\'')]
param_str = param_str[2:param_str.rfind("'")]

Comment on lines +111 to +114
>>> result_table = hyperleda.query_object(obj_name = 'UGC12591',
properties = 'objname,type,logr25,
btc,v,modbest,al2000,de2000,
celposJ(pgc)')
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor whitespace suggestion:

Suggested change
>>> result_table = hyperleda.query_object(obj_name = 'UGC12591',
properties = 'objname,type,logr25,
btc,v,modbest,al2000,de2000,
celposJ(pgc)')
>>> result_table = hyperleda.query_object(obj_name='UGC12591',
properties='objname,type,logr25,
btc, v, modbest, al2000, de2000,
celposJ(pgc)')

Comment on lines +149 to +156
request_payload = dict(n='meandata', c='o', of='1,leda,simbad',
nra='l', nakd='1',
d='{:}'.format(param_str),
sql='{:}'.format(ls_SQL_search), ob='',
a='csv[|]')
response = self._request("GET", url_http_request,
params=request_payload)
sql_result_tbl = Table.read(response.url, format='ascii', delimiter='|')
Copy link
Contributor

Choose a reason for hiding this comment

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

this code looks like it repeats query_sql below. Why not just return self.query_sql(search=ls_SQL_search, properties=properties)?

Comment on lines +260 to +267
# the default tool for users to interact with is an instance of the Class
hyperleda = HyperLEDAClass()

# once your class is done, tests should be written
# See ./tests for examples on this

# Next you should write the docs in astroquery/docs/module_name
# using Sphinx.
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the comments, and the capitalization should match that used in other modules (e.g., from astroquery.vizier import Vizier)

Suggested change
# the default tool for users to interact with is an instance of the Class
hyperleda = HyperLEDAClass()
# once your class is done, tests should be written
# See ./tests for examples on this
# Next you should write the docs in astroquery/docs/module_name
# using Sphinx.
HyperLEDA = HyperLEDAClass()

---------------

This module can be used to query from the HyperLEDA web service. The
queries will return the resultsin an astropy Table. Below are two
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
queries will return the resultsin an astropy Table. Below are two
queries will return the results in an astropy Table. Below are two

object,or using an SQL query request to the HyperLEDA SQL data access
service.

--------------
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
--------------


Query the object by name. For instance if you want to query UGC 1259

.. code:: ipython3
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is generic python and therefore should be just python

Suggested change
.. code:: ipython3
.. code:: python

celposJ(pgc) string -- Return the J2000 celestial position of object "pgc" in sexagesimal format


--------------
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think these -----'s separating sections are allowed? I'm not actually sure; how are they supposed to render?

@keflavich
Copy link
Contributor

Also, could you add a changelog entry?

@bsipocz bsipocz removed this from the v0.4.4 milestone Nov 13, 2021
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.

4 participants