Remove default wavelength limits from svo_fps get_filter_index()
#2509
Conversation
Previously the default behaviour of `get_filter_index()` from `svo_fps` was to make the wavelength range as large as possible, but that results in a timeout. Now the method requires the user to explicitly set the wavelength limits because it is better to receive an immediate `TypeError` about missing mandatory arguments than it is to wait a minute before an obscure timeout occurs. See also 90677c8
|
This is only a partial solution, I think - I just got a timeout for a rather reasonable query: from astroquery.svo_fps import SvoFps
index = SvoFps.get_filter_index(1000*u.AA, 1*u.um)It may be that we just need to increase the timeout, since there are apparently now too many optical filters? |
Codecov Report
@@ Coverage Diff @@
## main #2509 +/- ##
==========================================
- Coverage 62.99% 62.98% -0.01%
==========================================
Files 133 133
Lines 17293 17310 +17
==========================================
+ Hits 10893 10902 +9
- Misses 6400 6408 +8
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
A larger timeout limit makes it simpler to (accidentally) download very large files, so I'm not sure it would be a good idea to change the default. I would rather make the error message more helpful. |
|
One thing we can add to this is to query only astronomical, earth observing, or planetary missions. I believe that is through the |
|
Right, I agree - the problem seems to be that the error is simply a timeout, and there's not really a great reason for it. Even getting all the filters' metadata is only a few MB, but apparently the server has a hard time retrieving everything. |
|
Unfortunately, the |
|
Suggested changes look good to me, but maybe make a note about the timeout issue in the docs? |
There was a problem hiding this comment.
Adding one more note and example about acknowledging the timeout issue, to the docs would be great to be part of this fix. The rest of my comments are optional.
I recognize that the docs was skipped running prior this PR, so cleaning the doctest-skip all, as well as cleaning up the usage of **kwargs are optional extensions of the PR.
| @@ -80,19 +78,17 @@ def data_from_svo(self, query, cache=True, timeout=None, | |||
| # If no table element found in VOTable | |||
| raise IndexError(error_msg) | |||
|
|
|||
| def get_filter_index(self, wavelength_eff_min=0*u.angstrom, | |||
| wavelength_eff_max=FLOAT_MAX*u.angstrom, **kwargs): | |||
| def get_filter_index(self, wavelength_eff_min, wavelength_eff_max, **kwargs): | |||
There was a problem hiding this comment.
if you're interested in more cleanup here, there is no reason for using kwargs here, list them out instead, here and for the other methods, too.
There was a problem hiding this comment.
To be honest I am not interested in cleaning up svo_fps in the foreseeable future.
docs/svo_fps/svo_fps.rst
Outdated
| There are options to downselect based on the minimum | ||
| and maximum effective wavelength (``wavelength_eff_min`` | ||
| and ``wavelength_eff_max``, respectively). | ||
| .. doctest-skip-all |
There was a problem hiding this comment.
why skipping the rest? Instead skipping please refactor so that doctests are passed.
Maybe also add another example with a note that timeout issues are expected, but can be fixed with the passing a large timeout as kwarg.
There was a problem hiding this comment.
Would you prefer turning on testing for all the code examples to be done in a commit in this pull request or would a separate pull request be more appropriate?
There was a problem hiding this comment.
Having it here would be fine, after all this is a very short and non-complicated docs.
There was a problem hiding this comment.
I have turned on remote testing for more code blocks and ensured that the example output matches actual output. The plotting example is still being skipped, but it doesn't execute any astroquery code, so I'll leave it the way it is.
There was a problem hiding this comment.
all plotting examples should be using the .. plot:: directive at some point, or should be removed, but that's a scope outside of this PR.
If `get_filter_index()` fails because the server took too long to resolve the query then the resulting `TimeoutError` suggests making the wavelength range smaller, or increasing the timeout limit if a large range is needed. The documentation now also makes the same suggestions.
|
There are now suggestions about avoiding timeouts in both the error message that occurs if the server takes too long to respond, and in the narrative docs. |
Now the only code block in `svo_fps` docs that is not being tested at all is a plotting example, but it doesn't execute any `astroquery` code.
|
Thanks @eerovaher! |
So far the default behaviour of
get_filter_index()fromsvo_fpshas been to make the wavelength range as large as possible, but that results in a timeout. Now the method requires the user to explicitly set the wavelength limits. Although this is backwards incompatible, in practice the user already had to provide the limits and if they don't then it is better to receive an immediateTypeErrorabout missing mandatory arguments than it is to wait a minute before an obscure timeout occurs.See also 90677c8
Closes #2508