Alma documentation cleanup#1935
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1935 +/- ##
=======================================
Coverage 62.97% 62.97%
=======================================
Files 131 131
Lines 17073 17073
=======================================
Hits 10752 10752
Misses 6321 6321 📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
b363e19 to
bb62694
Compare
bsipocz
left a comment
There was a problem hiding this comment.
There are unrelated failures, ideally those should be resolved before merging this, to ensure the cleaned up docs indeed passes tests.
|
@keflavich - could you take over this PR to enable doctesting on the alma docs? |
|
I'm working on it, but I get this weird error: that is unrelated. This feels like something caused by an out-of-date astropy, but I'm testing w/ |
d5ba758 to
6fcc13d
Compare
|
There are two unrelated commits picked up. I'll look into how we could filter out that leap second warning, and also ping @eteq as this leap second thing comes up regularly, so there may be a solution for it that I cannot remember from the top of my head. |
|
@keflavich - That leap-second warning is already on the ignore list, so I'm not sure why it's causing issues locally. Did you see it before or after you made the rebase? |
62776ea to
70e53e8
Compare
|
I fixed the rebase - some weird things happened that I didn't understand. I had to rebase several times, even after updating the remote. I had to add a different warning catch for this warning: but I don't think that is right, something local must be broken again. |
70e53e8 to
dc408e7
Compare
| # This is a temporary measure, all of these should be fixed: | ||
| ignore::pytest.PytestUnraisableExceptionWarning | ||
| ignore::numpy.VisibleDeprecationWarning | ||
| ignore::DeprecationWarning |
There was a problem hiding this comment.
We shouldn't do this as we need to catch deprecations that we trigger with astroquery code.
There was a problem hiding this comment.
ach, yes, this was a mistake: for reasons I don't understand, I have to have this for tests to pass locally, but I didn't mean to commit it.
|
I'm getting a ton of exceptions like these, but they're totally unrelated to this PR. I think they're slowing down/screwing with the testing. |
|
I also see tons of these: but... why? We shouldn't have unclosed sockets, right? We're just doing a few requests? Maybe this is within |
|
I recall seeing those socket ones occasionally, but don't think it's related to the content of this PR, and suspect they are upstream either pytest or request. Do you recall seeing any from the python terminal, or they only pop up while you run the tests? |
|
I don't see them on the command line, just during tests. The doctests are super slow to run; I think they're downloading a lot of data. That might be necessary, but it certainly has made testing inconvenient. |
|
@keflavich - I also see that they run a very long time, though I don't have many files permanently downloaded. It would be good if the problematic ones would be rephrased, or at least skipped. I would prefer the rewrite as I think it's more useful for users if they can copy-paste examples that actually run and finishes within a reasonable time, they can then rescale it for their own use case. |
|
I think we should go ahead and merge this, but we might need a fresh PR to clean up some more details on another (future) review. Has the readthedocs build been pending since christmas? |
|
There is a problem with your build: https://readthedocs.org/projects/astroquery/builds/15892183/ |
|
@keflavich - does this build locally? As I recall it was hung up for me when I tried it before the holidays. We cannot merge if it hangs as it would block the rest of the package. |
|
I didn't realize there was a real build issue here. I'll investigate when I get back to this. |
|
This PR should also remove or refactor the test |
bsipocz
left a comment
There was a problem hiding this comment.
I'll try to rebase this and will squash a few commits to address my comments. Hopefully that will trigger RDT and it won't timeout on us any longer.
| >>> print(len(m83_data)) | ||
| 830 | ||
| >>> m83_data.colnames | ||
| >>> m83_data.colnames # doctest: +SKIP |
There was a problem hiding this comment.
This should be IGNORE_OUTPUT rather than skip
| >>> m83_data.colnames # doctest: +SKIP | |
| >>> m83_data.colnames # doctest: +IGNORE_OUTPUT |
| >>> from astroquery.alma import Alma | ||
| >>> m83_data = Alma.query_object('M83') | ||
| >>> uids = np.unique(m83_data['member_ous_uid']) | ||
| >>> print(uids) # doctest: +IGNORE_OUTPUT |
There was a problem hiding this comment.
Are these expected to change? If not, then we shall not ignore
| .. doctest-remote-data:: | ||
| >>> myAlma = Alma() # doctest: +SKIP | ||
| >>> myAlma.cache_location = '/big/external/drive/' # doctest: +SKIP | ||
| >>> myAlma.download_files(link_list, cache=True) # doctest: +SKIP |
There was a problem hiding this comment.
as all is skipped here, maybe leave it without remote data
57e63c8 to
6686a03
Compare
bsipocz
left a comment
There was a problem hiding this comment.
Passing locally, so we're good to go if the RTD rendering works.
e40cb4c to
9819ea9
Compare
|
Hmm, tests pass, but sphinx is still not happy, atm it's an IndexError: https://readthedocs.org/projects/astroquery/builds/16411376/ |
|
Apparently, two directives cannot be used at once... |
No description provided.