Fix_issue_2239: Nasa Exoplanet Archive query_object method suddenly produces InvalidTableError by rickynilsson · Pull Request #2264 · astropy/astroquery

rickynilsson · 2022-01-27T22:36:53Z

Fix query_aliases(). Now using aliaslookup service instead of deprecated aliastable.

pep8speaks · 2022-01-27T22:36:56Z

Hello @rickynilsson! 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 2022-03-04 00:40:02 UTC

bsipocz

I'm pretty sure that the test failure is coming from the URL assumption in the mock setup, if you test it locally it should fail there, too, not just for old versions.

bsipocz · 2022-01-27T23:27:29Z


    @class_or_instance
-    def query_aliases(self, object_name, *, cache=None):
+    def query_aliases(self, object_name):


why remove cache? If remove, clear out the docstring, too

Wasn't thinking, will add it back.

codecov · 2022-01-28T02:30:14Z

Codecov Report

Merging #2264 (8732c6d) into main (4e35238) will decrease coverage by 0.11%.
The diff coverage is 74.07%.

@@            Coverage Diff             @@
##             main    #2264      +/-   ##
==========================================
- Coverage   63.10%   62.98%   -0.12%     
==========================================
  Files         131      131              
  Lines       17040    17059      +19     
==========================================
- Hits        10753    10745       -8     
- Misses       6287     6314      +27

Impacted Files	Coverage Δ
...roquery/ipac/nexsci/nasa_exoplanet_archive/core.py	`60.76% <74.07%> (-8.12%)`	⬇️

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 4e35238...8732c6d. Read the comment docs.

bsipocz

This fix needs to be mentioned in the changelog as the bug was reported by users. (And also add a mention of the API change for the removed keyword).

Other than the minor comments I left in the review, there is one remote test failing, maybe we could test not for the exact number of aliases but that there are e.g. more than 10?
And if you run the remote test, they would also reveal some docs example inconsistencies. For some of those, e.g. where we expect changes as the upstream database grows, I would suggest to use # doctest: +IGNORE_OUTPUT.

bsipocz · 2022-01-28T16:35:05Z

-                "aliastable", objname=object_name.strip(), cache=cache, format="csv"
-            )["aliasdis"]
-        )
+        url = requests.get("https://exoplanetarchive.ipac.caltech.edu/cgi-bin/Lookup/nph-aliaslookup.py?objname="+object_name)


As you added the base URL for the lookup to the config, please us it from there.

bsipocz · 2022-01-28T16:38:34Z


-def test_regularize_object_name(patch_get):
-    NasaExoplanetArchiveMock = NasaExoplanetArchiveClass()
+# def test_regularize_object_name(patch_get):


Please either uncomment or if the regularize keyword is removed, then remove this test, too.

bsipocz · 2022-01-28T16:43:00Z

    @deprecated(since="v0.4.1", alternative="query_object")
    @deprecated_renamed_argument(["show_progress", "table_path"],
                                 [None, None], "v0.4.1", arg_in_kwargs=True)
-    def query_star(self, host_name, cache=None, regularize=True, **criteria):


This API change needs to be mentioned in the changelog

rickynilsson · 2022-01-28T19:18:34Z

@bsipocz - I fixed up the testing and other things you mentioned, but seems codecov isn't happy. Any suggested tests I could add? I actually am not quite sure why the coverage isn't higher -- the tests should be running most of the added code.

bsipocz · 2022-01-28T19:31:17Z

@rickynilsson - I'll need to look into this more carefully to suggest actual code changes, but at first glance, it seems that the mock testing is not actually running anything from the real method from the module, but only the mock function defined in the test, and that triggers the decrease in the coverage number.

bsipocz · 2022-03-01T05:30:53Z

@rickynilsson - do you plan to add some extra test coverage here as part of the PR, or we should rather go ahead and merge and add more tests in a follow-up?

Before merging: doc examples should be fixed (if you run python setup.py -P ipac.nexsci -R it picks up the docs file, too), and a rebase should fix the changelog conflict.

rickynilsson · 2022-03-01T05:36:12Z

I should probably just make a final push on this to get the necessary mock tests in place. I'll set off some time to do this tomorrow.

rickynilsson · 2022-03-02T01:06:49Z

@bsipocz - something is obviously wrong in the mock test I added but I can't figure out exactly what. Any hints? The monkey patching stuff is a bit confusing to me.

bsipocz

There is a super nice 92% coverage, with and another nice 83% remote only coverage, so I'm fairly certain we got the majority of use cases under control :)

…ted aliastable.

…bject_name. Fix doctest issues by adding +IGNORE OUTPUT. Fix remote test of query_aliases().

bsipocz · 2022-03-04T00:50:36Z

Thank you @rickynilsson!

bsipocz added ipac.nexsci.nasa_exoplanet_archive Upstream labels Jan 27, 2022

bsipocz reviewed Jan 27, 2022

View reviewed changes

bsipocz reviewed Jan 28, 2022

View reviewed changes

bsipocz approved these changes Mar 4, 2022

View reviewed changes

Comment thread astroquery/ipac/nexsci/nasa_exoplanet_archive/tests/test_nasa_exoplanet_archive.py Outdated

rickynilsson added 15 commits March 3, 2022 16:37

Fix query_aliases(). Now using aliaslookup service instead of depreca…

137ffcb

…ted aliastable.

Fix codestyle issues.

530c050

Add back cache.

28d6991

Fix tests.

ab1f117

Fix tests.

5a88a03

Fix codestyle

bf33cf7

Get base URL from config via get_access_url. Remove test_regularize_o…

4880322

…bject_name. Fix doctest issues by adding +IGNORE OUTPUT. Fix remote test of query_aliases().

Add updates to changelog.

ffa8217

Fix title underline.

d5b3b19

Add mock test

1476f96

Add mock data file

cfea290

Add remote data decorator

5b8d3e7

Add more tests. Fix missing +IGNORE_OUTPUT in doc. Fix typo.

d13f6aa

Fixed mock test

2acad05

Remove whitespace

8732c6d

bsipocz force-pushed the fix_issue_2239 branch from 9da2e1c to 8732c6d Compare March 4, 2022 00:39

bsipocz merged commit 3342888 into astropy:main Mar 4, 2022

Uh oh!

Uh oh!

Conversation

rickynilsson commented Jan 27, 2022

Uh oh!

pep8speaks commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2022-03-04 00:40:02 UTC

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

bsipocz Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

rickynilsson Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

bsipocz Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

bsipocz Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

bsipocz Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

rickynilsson commented Jan 28, 2022

Uh oh!

bsipocz commented Jan 28, 2022

Uh oh!

bsipocz commented Mar 1, 2022

Uh oh!

rickynilsson commented Mar 1, 2022

Uh oh!

rickynilsson commented Mar 2, 2022

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bsipocz commented Mar 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pep8speaks commented Jan 27, 2022 •

edited

Loading

codecov Bot commented Jan 28, 2022 •

edited

Loading