-
-
Notifications
You must be signed in to change notification settings - Fork 394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MAINT: Fixing HEASARC and IMCCE docs issue #2652
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2652 +/- ##
=======================================
Coverage 68.94% 68.94%
=======================================
Files 304 304
Lines 22621 22621
=======================================
Hits 15596 15596
Misses 7025 7025
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Checks passed for HEASARC changes, so I'll go after the imcce and mast doc fixes before asking for review. |
MAST seems to have been already been addressed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
@@ -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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested the cone search with np 1.23 and 1.24 versions, so I'll commit and keep working through the failures with imcce.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I'm eager to help out wherever I can! |
mast.rst still passes doctests for me, so I'll leave it unless there's anything more specific you'll want me to do. |
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. |
docs/heasarc/heasarc.rst
Outdated
@@ -264,15 +264,31 @@ tables listing most recent INTEGRAL data. | |||
... radius='361 degree', time="2021-02-01 .. 2030-12-01", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
You can always rebase/squash things here (or I can do that prior merging), that would totally things it up. |
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. |
6071203
to
942673b
Compare
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
This seems to be a broader issue that is already known (#2349). |
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 |
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 |
Thanks @nkphysics! |
Updated the HEASARC docs issues addressed in #2634 .
I'm new so I thought I'd just try out something small before trying more.