Revert kwarg-only function arguments for splatalogue #2696

keflavich · 2023-03-31T22:23:02Z

Splatalogue was modified to accept only keyword-specified arguments, but Splatalogue.query_lines() is not valid. Currently, all of the docs have invalid examples as a result. This simple modification fixes the problem. The standard usage is, and should be:

Splatalogue.query_lines(min_frequency, max_frequency)

the alternative, which is presently being enforced, should be optional:

Splatalogue.query_lines(min_frequency=min_frequency, max_frequency=max_frequency)

There's another optional approach that is supported either way:

Splatalogue.query_lines(band='alma3')

codecov · 2023-03-31T22:30:27Z

Codecov Report

Merging #2696 (8c24bb6) into main (529721b) will increase coverage by 0.00%.
The diff coverage is 87.50%.

@@           Coverage Diff           @@
##             main    #2696   +/-   ##
=======================================
  Coverage   65.73%   65.74%           
=======================================
  Files         233      233           
  Lines       17842    17844    +2     
=======================================
+ Hits        11729    11732    +3     
+ Misses       6113     6112    -1

Impacted Files	Coverage Δ
astroquery/splatalogue/load_species_table.py	`89.47% <0.00%> (ø)`
astroquery/splatalogue/core.py	`87.82% <100.00%> (+0.80%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

bsipocz · 2023-03-31T23:11:49Z

Hmm, the docs still has the doctest-skip-all, thus we haven't notice this before.

Why do you feel strongly against requiring being explicit? E.g. in practice all the parameters are optional as you either need the min/max or the bandname, so just assuming that if two are given they would be min/max is not exactly great.

So, what about updating the docs instead, and enabling remote testing on it?

keflavich · 2023-04-01T14:31:38Z

The band name should be regarded as optional, imo, and the min/max frequency should not. They are only technically optional, in the literal sense of a query can be executed without raising an immediate error if you give a band name. You cannot execute successful queries without providing either a tight pair of frequencies or a band name plus a chemical name.

This is a module I'm intimately familiar with, and I believe Splatalogue.query_lines(1*u.GHz, 2*u.GHz) is both the most common and most appropriate pattern. I'd be much happier deprecating/blocking the 'band' query and making min_frequency, max_frequency required non-keyword arguments than leaving the module as is. query_lines(minfreq, maxfreq) is much closer in spirit to query_object(name) or query_region(coordinates) than it is to a generic query(**kwargs) function.

I did some searching for use cases on github, too, and all uses I saw (that were not astroquery clones or my own test cases....) use positional arguments.

https://github.com/search?p=4&q=splatalogue.query_lines&type=Code

That said, I do think we should update the docs to use explicit keywords in all examples.

keflavich · 2023-04-01T16:02:25Z

I don't understand the readthedocs failure. I'm not sure if it's related or not; the traceback makes no sense.

keflavich · 2023-04-02T22:44:00Z

The error messages were nonsense, but were caused by doubling up sphinx block directives

bsipocz · 2023-04-03T15:33:16Z

The error messages were nonsense, but were caused by doubling up sphinx block directives

Interesting, this never caused an issue before, the doubling up just caused an ugly rendering.

keflavich · 2023-04-03T15:34:22Z

Maybe it was because I put them on consecutive lines? The message never told me what I was doing was wrong specifically, but it caused errors in every other module.

bsipocz · 2023-04-03T15:38:52Z

We had them in consecutive lines in all the other PRs that were opted in for the testing, but most of those was a while ago and likely used older sphinx. I haven't checked, but it might become more strict. Anyway, I'm glad you find the fix.

bsipocz · 2023-04-03T15:41:42Z

This is a module I'm intimately familiar with, and I believe Splatalogue.query_lines(1u.GHz, 2u.GHz) is both the most common and most appropriate pattern. I'd be much happier deprecating/blocking the 'band' query and making min_frequency, max_frequency required non-keyword arguments than leaving the module as is. query_lines(minfreq, maxfreq) is much closer in spirit to query_object(name) or query_region(coordinates) than it is to a generic query(**kwargs) function.

OK. I still don't like it from the pure API design POV, but could live with the two positional arguments. I would not think making them mandatory would make things better, the band kward feels very useful.

keflavich · 2023-04-03T18:30:18Z

The main change in this PR is this one:

astroquery/astroquery/splatalogue/core.py

Line 422 in 86ec38f

def query_lines_async(self, min_frequency=None, max_frequency=None, *,

so I kept the arguments as optional positional arguments.

I also made regex_str:

astroquery/astroquery/splatalogue/core.py

Line 79 in 86ec38f

def get_species_ids(self, regex_str=None, *, reflags=0, recache=False):

a positional argument, since the intent of that function is to search.

Otherwise, this is all documentation fixes. It's ready for review.

bsipocz

OK, some more comments. The main blocker is the kwarg rename, it should be done with a deprecation decorator.

And please add a changelog about the rename, too.

bsipocz · 2023-04-03T18:43:39Z

astroquery/splatalogue/core.py

-    def get_species_ids(self, *, restr=None, reflags=0, recache=False):
+    def get_species_ids(self, regex_str=None, *, reflags=0, recache=False):


This renaming of an arg is an API change that has to be done with the deprecation decorator. Unless restr was added since 0.4.6.

and if rename happens, you may want to use a name that's more informative. Maybe species_regex?

OK, I'm willing to abandon that. restr wasn't great, it was just short. It... kind of was never intended to be a kwarg, just an internal required positional variable.

haha, good reminder that internal stuff should be named well, too. Or at least the stuff that ends up in signatures and docstrings.
I'm happy to push commits to this, but it will need to wait until at least the evening.

yeah I will revisit this when I can; may not be this evening either

I renamed the variable, but I still prefer to keep it as a positional argument. I added the deprecation warning.

bsipocz · 2023-04-03T19:04:47Z

astroquery/splatalogue/load_species_table.py

@@ -66,10 +66,12 @@ def species_lookuptable(*, filename='splat-species.json', recache=False):
    # check to see if the file exists; if not, we run the
    # scraping routine
    if recache or not os.path.isfile(file_cache):
-        species = get_json_species_ids(filename)
+        species = get_json_species_ids(outfile=filename)


there are some patchy test coverage around this load/build methods, and even __main__, so would be nice to clean up that, too (can be done separately, I've just spotted it now).

yes, agreed - I already expanded test coverage a bit, I think? but, some stuff is still not caught

I've not done anything with this but suggest we merge anyway.

astroquery/splatalogue/build_species_table.py

astroquery/splatalogue/core.py

bsipocz

More comments regarding the documentation, basically the asserts should go, but otherwise things are looking good.

bsipocz · 2023-04-12T18:12:45Z

docs/splatalogue/splatalogue.rst

@@ -1,5 +1,3 @@
-.. doctest-skip-all


Interesting why the examples work in this without remote-data? Would it make sense to make it opt-out to use the shipped json?

Many of them are labeled remote-data; only a couple work without it.

docs/splatalogue/splatalogue.rst

keflavich · 2023-04-12T22:21:24Z

@bsipocz any idea what's going on with the dev deps build? Looks like pyvo and codecov don't get along?

bsipocz · 2023-04-12T22:23:13Z

any idea what's going on with the dev deps build? Looks like pyvo and codecov don't get along?

Ahh, so codecov actually bite us. cc @pllim
I'll fix this separately.

fix typo

refactor of some broken code and renames some variables for clarity. The assertions are added because one test case resulted in a supposedly-json file being read in as a string, which resulted in breakage downstream

bsipocz

When CI is all clean, this is good to go.

keflavich force-pushed the splatalogue_args branch from f384e3c to c200b49 Compare March 31, 2023 22:25

keflavich force-pushed the splatalogue_args branch from 72ec6b6 to 3003a02 Compare April 1, 2023 15:50

bsipocz requested changes Apr 3, 2023

View reviewed changes

bsipocz added the splatalogue label Apr 12, 2023

bsipocz added this to the v0.4.7 milestone Apr 12, 2023

bsipocz added the no-changelog-entry-needed label Apr 12, 2023

bsipocz reviewed Apr 12, 2023

View reviewed changes

keflavich and others added 9 commits April 12, 2023 18:17

revert kwarg-only definitions for splatalogue

149e87f

fix typo

fix documentation; doctests now pass. This PR includes significant

cd64109

refactor of some broken code and renames some variables for clarity. The assertions are added because one test case resulted in a supposedly-json file being read in as a string, which resulted in breakage downstream

fix change of formaldehyde species ID

6aaab6d

remove example that is no longer included

1f64184

can't have both doctest-remote-data and code-block directives

78274f5

change 'regex_str' to 'species_regex'

82ef89b

add deprecation decorator

ba3204e

remove asserts from doctests

41864d4

Remove asserts from code

8c24bb6

bsipocz force-pushed the splatalogue_args branch from c8c3b64 to 8c24bb6 Compare April 13, 2023 01:19

bsipocz approved these changes Apr 13, 2023

View reviewed changes

bsipocz merged commit ac341f3 into astropy:main Apr 13, 2023

		def get_species_ids(self, *, restr=None, reflags=0, recache=False):
		def get_species_ids(self, regex_str=None, *, reflags=0, recache=False):

Uh oh!

Revert kwarg-only function arguments for splatalogue #2696

Revert kwarg-only function arguments for splatalogue #2696

Uh oh!

Conversation

keflavich commented Mar 31, 2023

Uh oh!

codecov bot commented Mar 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bsipocz commented Mar 31, 2023

Uh oh!

keflavich commented Apr 1, 2023

Uh oh!

keflavich commented Apr 1, 2023

Uh oh!

keflavich commented Apr 2, 2023

Uh oh!

bsipocz commented Apr 3, 2023

Uh oh!

keflavich commented Apr 3, 2023

Uh oh!

bsipocz commented Apr 3, 2023

Uh oh!

bsipocz commented Apr 3, 2023

Uh oh!

keflavich commented Apr 3, 2023

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keflavich commented Apr 12, 2023

Uh oh!

bsipocz commented Apr 12, 2023

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Mar 31, 2023 •

edited

Loading