Develop catalogues#6
Conversation
astroquery/eso/__init__.py
Outdated
| @@ -19,7 +19,9 @@ class Conf(_config.ConfigNamespace): | |||
| tap_url = _config.ConfigItem( | |||
There was a problem hiding this comment.
Rename tap_url --> tap_obs_url and its explanation as well, so that it is symmetrical with the config entry for catalogues.
astroquery/eso/core.py
Outdated
| row_limit_plus_one = self.ROW_LIMIT + 1 | ||
|
|
||
| table_with_an_extra_row = tap.search(query=query_str, maxrec=row_limit_plus_one).to_table() | ||
| table_with_an_extra_row = tap.search(query=query_str, |
There was a problem hiding this comment.
Note to myself: Either this 4-lines change 1) has no effect; 2) is wrong; or 3) the previous code is wrong. Double check...
There was a problem hiding this comment.
@ashleythomasbarnes, you can safely discard these changes (i.e. keep the original code). Explanation:
The proposed code change has no effect. It seems that the AI didn't understand the purpose of the if clause, and it was natural for it to turn it into a if...else clause.
The original if clause is only a safeguard against increasing an infinite row limit. It was introduced to fix a bug appearing when the user set ROW_LIMIT = 0. If we want to sound very mathematically elegant, we can say that it simply derives from the fact that ∞ +1 = ∞
astroquery/eso/core.py
Outdated
| ) | ||
|
|
||
| log.debug(f"Querying from {self._tap_url()}") | ||
| tap_url = self._tap_url() if which_tap == "tap_obs" else conf.tap_cat_url |
There was a problem hiding this comment.
here... This is another instance of the situation not being symmetrical between tap_cat and tap_obs: One is read from self._tap_url(); the other from the config directly. It is OK to have tap_obs set as a default, but for the rest tap_cat and tap_obs should be symmetrical. Do we have a good argument for them NOT being treated the same?
astroquery/eso/core.py
Outdated
| f"\nNumber of records present in the table {table_name}:\n{num_records}\n") | ||
|
|
||
| @unlimited_maxrec | ||
| @deprecated_renamed_argument('cache', None, since='0.4.12') |
There was a problem hiding this comment.
No need for deprecating anything here, since it is a new function. We can remove the cache argument.
astroquery/eso/utils.py
Outdated
| from dataclasses import dataclass | ||
| from typing import Dict, List, Optional, Union | ||
|
|
||
| import numpy as np |
There was a problem hiding this comment.
Numpy is not used. This line should be removed.
juanmcloaiza
left a comment
juanmcloaiza
left a comment
There was a problem hiding this comment.
Looks good! Only one change needed (about sys.maxsize), and one would-be-nice (about which_tap and tap_url) -- see the comments to the code.
The tests are still failing, but as soon as they pass, the PR can be merged.
astroquery/eso/core.py
Outdated
| table_with_an_extra_row = Table() | ||
|
|
||
| def message(query_str): | ||
| which_tap = tap.baseurl.split("/")[-1] |
There was a problem hiding this comment.
This looks hacky. It works, but it doesn't give us freedom to define a new "which_tap" key or rename the existing ones, if they don't follow exactly this convention. I'd propose to create a very small helper function that is the inverse of _tap_url. Example:
# This one exists:
def _tap_url(self, which_tap: str = "tap_obs") -> str:
# Returns the URL given the which_tap label
# Raises if the label is not recognized
# Define this one, the inverse:
def _which_tap(self, tap_url: str = conf.tap_obs_url) -> str:
# Returns the which_tap label given a URL
# Raises if the URL is not recognized
Not a hard requirement to merge the PR, since this is the only instance where we need to recover which_tap from the tap_url, but it would make the code tidier.
ashleythomasbarnes
changed the base branch from
develop
to
develop-catalogues
2 participants
New PR from #4 to help @juanmcloaiza for review...
Stripped back now, only query_catalogue and list_catalogues that mirror functionality of other functions included.