MAINT: Fixing HEASARC and IMCCE docs issue #2652

nkphysics · 2023-01-26T23:44:49Z

Updated the HEASARC docs issues addressed in #2634 .

I'm new so I thought I'd just try out something small before trying more.

codecov · 2023-01-26T23:51:57Z

Codecov Report

Merging #2652 (6071203) into main (d2d6529) will not change coverage.
The diff coverage is n/a.

❗ Current head 6071203 differs from pull request most recent head 942673b. Consider uploading reports for the commit 942673b to get more accurate results

@@           Coverage Diff           @@
##             main    #2652   +/-   ##
=======================================
  Coverage   68.94%   68.94%           
=======================================
  Files         304      304           
  Lines       22621    22621           
=======================================
  Hits        15596    15596           
  Misses       7025     7025

Impacted Files	Coverage Δ
astroquery/utils/commons.py	`75.00% <0.00%> (ø)`

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

nkphysics · 2023-01-26T23:57:32Z

Checks passed for HEASARC changes, so I'll go after the imcce and mast doc fixes before asking for review.

nkphysics · 2023-01-27T00:40:07Z

MAST seems to have been already been addressed

bsipocz

Hi @nkphysics 👋 Thanks for the PR. I'm sorry for harvesting the lowest hanging ones of these remote tests failures earlier today.
If you would like to start with something small, or easily scalable, there are a few issues that has suggestions for refactorings affecting all the modules, so those are super suitable for starters (e.g. #1746, but there are others too).

bsipocz · 2023-01-27T01:16:30Z

docs/imcce/imcce.rst

@@ -38,23 +38,15 @@ looks like this:
   >>> field = SkyCoord(0*u.deg, 0*u.deg)
   >>> epoch = Time('2019-05-29 21:42', format='iso')
   >>> Skybot.cone_search(field, 5*u.arcmin, epoch)
-    <QTable length=12>


This example seems to be very dependent on the versions (e.g. np 1.23 vs 1.24), so a bit more robust solution will be needed here.

Would this be worth an IGNORE_INPUT then?
Like with HEASARC I just got too used to having broken up tests and just spotted 1 issue in the action logs so just handled the one.
I have fixes I can commit for imccre using np 1.24, but I'll need to mess with 1.23 if necessary.

Tested the cone search with np 1.23 and 1.24 versions, so I'll commit and keep working through the failures with imcce.

Ok so after looking at a few of the other docs, a common trend I'm seeing to workaround this is using pprint.
So in this specific case it might look something like this...

>>> search = Skybot.cone_search(field, 5*u.arcmin, epoch) >>> search.pprint(max_width=100) Number Name RA DEC ... vy vz epoch deg deg ... AU / d AU / d d ------ ---------- -------------------- --------------------- ... ----------- ----------- --------- -- 2012 BO42 0.019414999999999998 -0.0128425 ... 0.009345668 0.005003011 2458630.0 516566 2007 DH36 0.005546249999999999 0.023103333333333333 ... 0.00855646 0.002875928 2458630.0 -- 2019 SS82 359.9931945833333 -0.028837777777777778 ... 0.009809784 0.004636687 2458630.0 163149 2002 CV106 359.98692374999996 -0.06923777777777777 ... 0.009078104 0.00267749 2458630.0

Probably not the best workaround, but just an idea for now.

using pprint sounds OK to me, but if I can nitpick call it results, not search. (And I would still expect it may be version sensitive given the primary issue was with the number of rows returned)

The output can just be restricted with say...

>>> results = Skybot.cone_search(field, 5*u.arcmin, epoch) >>> results[:4]pprint(max_width=100)

I only use 4 here out of convenience.
Similar row output restrictions will need to be made with the other examples for imcce.
For example with Miride the example could be changed so that it will query ephemerides for asteroid
Pallas over an entire month, or maybe week (rather than year) with a time step of 1 day.
And the output can be restricted to the corresponding scale.

Although the version sensitivity seems to be a broader issue with the docs.
For example from the docs for eso:

eso.ROW_LIMIT = -1 # Return all results table = eso.query_main(column_filters={'instrument': 'APICAM', 'filter_path': 'LUMINANCE', 'stime':'2019-04-26', 'etime':'2019-04-27'}, cache=False) print(len(table)) 207 print(table.columns) <TableColumns names=('OBJECT','RA','DEC','Program_ID','Instrument','Category','Type','Mode','Dataset ID','Release_Date','TPL ID','TPL START','Exptime','Exposure','filter_lambda_min','filter_lambda_max','MJD-OBS','Airmass','DIMM Seeing at Start')> table.pprint(max_width=100) OBJECT RA DEC Program_ID ... MJD-OBS Airmass DIMM Seeing at Start ------- ----------- ----------- ------------ ... ------------ ------- -------------------- ALL SKY 09:18:37.39 -24:32:32.7 60.A-9008(A) ... 58599.987766 1.0 N/A ALL SKY 09:21:07.68 -24:32:30.1 60.A-9008(A) ... 58599.989502 1.0 N/A ALL SKY 09:23:38.98 -24:32:27.5 60.A-9008(A) ... 58599.99125 1.0 N/A ALL SKY 09:26:10.28 -24:32:24.9 60.A-9008(A) ... 58599.992998 1.0 N/A ALL SKY 09:28:40.58 -24:32:22.4 60.A-9008(A) ... 58599.994734 1.0 N/A ALL SKY 09:31:43.93 -24:32:19.4 60.A-9008(A) ... 58599.996852 1.0 N/A ALL SKY 09:34:15.23 -24:32:17.0 60.A-9008(A) ... 58599.9986 1.0 N/A ALL SKY 09:36:47.53 -24:32:14.5 60.A-9008(A) ... 58600.000359 1.0 N/A ALL SKY 09:39:18.82 -24:32:12.2 60.A-9008(A) ... 58600.002106 1.0 N/A ALL SKY 09:41:49.11 -24:32:09.9 60.A-9008(A) ... 58600.003843 1.0 N/A ... ... ... ... ... ... ... ... ALL SKY 19:07:39.21 -24:39:35.1 60.A-9008(A) ... 58600.395914 1.0 N/A ALL SKY 19:10:11.68 -24:39:39.1 60.A-9008(A) ... 58600.397674 1.0 N/A ALL SKY 19:12:44.15 -24:39:43.2 60.A-9008(A) ... 58600.399433 1.0 N/A ALL SKY 19:15:15.62 -24:39:47.1 60.A-9008(A) ... 58600.401181 1.0 N/A ALL SKY 19:17:46.09 -24:39:51.1 60.A-9008(A) ... 58600.402917 1.0 N/A ALL SKY 19:20:46.65 -24:39:55.8 60.A-9008(A) ... 58600.405 1.0 N/A ALL SKY 19:23:18.12 -24:39:59.7 60.A-9008(A) ... 58600.406748 1.0 N/A ALL SKY 19:25:51.60 -24:40:03.7 60.A-9008(A) ... 58600.408519 1.0 N/A ALL SKY 19:28:22.08 -24:40:07.6 60.A-9008(A) ... 58600.410255 1.0 N/A ALL SKY 19:30:52.55 -24:40:11.4 60.A-9008(A) ... 58600.411991 1.0 N/A Length = 207 rows

Cut the outputs with ellipsis rather than in the command itself. We tend to keep 3-5 first and last lines.

As for version-dependent stuff, I would just avoid changing the outputs for now, and would rather try to find the underlying reason for the changes.

docs/heasarc/heasarc.rst

nkphysics · 2023-01-27T04:21:06Z

Hi @nkphysics wave Thanks for the PR. I'm sorry for harvesting the lowest hanging ones of these remote tests failures earlier today. If you would like to start with something small, or easily scalable, there are a few issues that has suggestions for refactorings affecting all the modules, so those are super suitable for starters (e.g. #1746, but there are others too).

I'm eager to help out wherever I can!

nkphysics · 2023-01-27T16:00:29Z

MAST seems to have been already been addressed

mast.rst still passes doctests for me, so I'll leave it unless there's anything more specific you'll want me to do.

bsipocz · 2023-01-27T16:41:01Z

I've seen similar issues with mast as with IMCCE, that it was version/and or way of running the tests dependent how the printing of the masked cells was done. So I would leave that out for now.

nkphysics · 2023-01-30T21:15:02Z

docs/heasarc/heasarc.rst

@@ -264,15 +264,31 @@ tables listing most recent INTEGRAL data.
    ...                              radius='361 degree', time="2021-02-01 .. 2030-12-01",


The Integral science window database table is still being updated so the timeframe in this query should probably be modified so that new updates to the database table don't constantly change this output.
https://heasarc.gsfc.nasa.gov/W3Browse/integral/intscw.html shows it was last updated today.

bsipocz · 2023-01-30T21:51:40Z

Would it be better if I rebase or close this PR and open separate, cleaner PR's for each?
Cause I'm making a mess of this

You can always rebase/squash things here (or I can do that prior merging), that would totally things it up.

nkphysics · 2023-01-31T00:19:38Z

The heasarc docs "should" be good to go with the changes I made to the alternative server example. Unless the folks with the integral mission change their table for 2022-12 the example "should" hold up to the tests.

nkphysics · 2023-02-01T22:41:08Z

I went ahead and rebased after making the pprint changes to the imcce Skybot.cone_search example.

When running through the deps and testing version-dependency imcce is passing no problem with older deps, leading me to believe the previous failures with imcce are due to changes on imcce's side. A more long term fix may be to use IGNORE_OUTPUT.
Heasarc passes with the present changes, however will fail with some older versions due to the following warning:

WARNING: UnitsWarning: 'MJD' did not parse as fits unit: At col 0, Unit 'MJD' not supported by the FITS standard. Did you mean MJ or mJ? If this is meant to be a custom unit, define it with 'u.def_unit'. To have it recognized inside a file reader or other code, enable it with 'u.add_enabled_units'. For details, see https://docs.astropy.org/en/latest/units/combining_and_defining.html [astropy.units.core]

This seems to be a broader issue that is already known (#2349).
On some other projects's I've work on where astroquery is a dep, these kind of warnings were just ignored since the output was still something that could be used, but I'm pretty sure you don't want me ignoring those kind of messages here.

bsipocz · 2023-02-01T23:57:07Z

On some other projects's I've work on where astroquery is a dep, these kind of warnings were just ignored since the output was still something that could be used, but I'm pretty sure you don't want me ignoring those kind of messages here.

This is fine here, I would not ignore the warning unless necessary. We'll always have some remote failures so it won't cause a green status to be red, and don't really run remote tests for all types of versions. If it becomes too annoying for local testing, we can add a # doctest: +IGNORE_WARNINGS to that line.

bsipocz · 2023-02-02T02:50:39Z

Sometimes I still see that mask array issue, but cannot pinpoint it down to anything, I couldn't make it fail consistently (and most recently all my tries are passing), which makes no sense at all. Anyway, we will just roll with what we have atm. I would say not to add the ignore just yet, but try to learn what exactly the issue is

bsipocz · 2023-02-02T02:51:41Z

Thanks @nkphysics!

nkphysics marked this pull request as ready for review January 27, 2023 00:40

nkphysics changed the title ~~MAINT: Updated HEASARC docs issue~~ MAINT: Updated HEASARC and IMCCE docs issue Jan 27, 2023

nkphysics changed the title ~~MAINT: Updated HEASARC and IMCCE docs issue~~ MAINT: Fixing HEASARC and IMCCE docs issue Jan 27, 2023

bsipocz reviewed Jan 27, 2023

View reviewed changes

nkphysics commented Jan 30, 2023

View reviewed changes

bsipocz added Documentation testing no-changelog-entry-needed labels Jan 30, 2023

bsipocz added this to the v0.4.7 milestone Jan 30, 2023

nkphysics added 2 commits February 1, 2023 16:15

DOC: Updated HEASARC docs

68d3e96

DOC: Updated IMCCE docs

942673b

nkphysics force-pushed the fixing_docs_23Q1 branch from 6071203 to 942673b Compare February 1, 2023 22:17

bsipocz approved these changes Feb 2, 2023

View reviewed changes

bsipocz merged commit e4612b4 into astropy:main Feb 2, 2023

nkphysics deleted the fixing_docs_23Q1 branch February 2, 2023 03:05

bsipocz mentioned this pull request Feb 3, 2023

BUG: failing remote tests Q1 2023 #2634

Closed

19 tasks

This was referenced Feb 10, 2023

Refactor: Make kwargs keyword only #2661

Merged

Cadc and Heasarc Kwarg refactors #2671

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAINT: Fixing HEASARC and IMCCE docs issue #2652

MAINT: Fixing HEASARC and IMCCE docs issue #2652

nkphysics commented Jan 26, 2023

codecov bot commented Jan 26, 2023 •

edited

Loading

nkphysics commented Jan 26, 2023

nkphysics commented Jan 27, 2023

bsipocz left a comment

bsipocz Jan 27, 2023

nkphysics Jan 27, 2023

nkphysics Jan 27, 2023

nkphysics Jan 28, 2023

bsipocz Jan 30, 2023

nkphysics Jan 30, 2023

bsipocz Jan 30, 2023

nkphysics commented Jan 27, 2023

nkphysics commented Jan 27, 2023

bsipocz commented Jan 27, 2023

nkphysics Jan 30, 2023

bsipocz commented Jan 30, 2023

nkphysics commented Jan 31, 2023

nkphysics commented Feb 1, 2023

bsipocz commented Feb 1, 2023

bsipocz commented Feb 2, 2023 •

edited

Loading

bsipocz commented Feb 2, 2023

		@@ -264,15 +264,31 @@ tables listing most recent INTEGRAL data.
		... radius='361 degree', time="2021-02-01 .. 2030-12-01",

MAINT: Fixing HEASARC and IMCCE docs issue #2652

MAINT: Fixing HEASARC and IMCCE docs issue #2652

Conversation

nkphysics commented Jan 26, 2023

codecov bot commented Jan 26, 2023 • edited Loading

Codecov Report

nkphysics commented Jan 26, 2023

nkphysics commented Jan 27, 2023

bsipocz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkphysics commented Jan 27, 2023

nkphysics commented Jan 27, 2023

bsipocz commented Jan 27, 2023

Choose a reason for hiding this comment

bsipocz commented Jan 30, 2023

nkphysics commented Jan 31, 2023

nkphysics commented Feb 1, 2023

bsipocz commented Feb 1, 2023

bsipocz commented Feb 2, 2023 • edited Loading

bsipocz commented Feb 2, 2023

codecov bot commented Jan 26, 2023 •

edited

Loading

bsipocz commented Feb 2, 2023 •

edited

Loading