Skip to content
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

ESA_JWST-doc-update: actual examples in JWST module #2451

Merged
merged 8 commits into from
Jun 28, 2022

Conversation

jespinosaar
Copy link
Contributor

Dear astroquery team:

This PR aims to update the documentation in ESA JWST module with more actual examples. I hope it is simple to merge. Thanks in advance for your support!

cc @esdc-esac-esa-int

@jespinosaar jespinosaar added this to the v0.4.7 milestone Jun 23, 2022
@jespinosaar jespinosaar self-assigned this Jun 23, 2022
@jespinosaar
Copy link
Contributor Author

Hi @bsipocz , we have only updated the documentation, without changing the code, it is necessary that we include a row in the changelog? or maybe we can merge this PR without this change? thanks in advance!

@codecov
Copy link

codecov bot commented Jun 23, 2022

Codecov Report

Merging #2451 (b38cd62) into main (05f65ea) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2451   +/-   ##
=======================================
  Coverage   62.92%   62.92%           
=======================================
  Files         133      133           
  Lines       17300    17300           
=======================================
  Hits        10886    10886           
  Misses       6414     6414           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@bsipocz
Copy link
Member

bsipocz commented Jun 23, 2022

No, we don't do changelog for docs or any non user facing internal changes.

@jespinosaar
Copy link
Contributor Author

Then, if there are not any other comments, maybe we can merge this PR.

docs/esa/jwst/jwst.rst Outdated Show resolved Hide resolved
docs/esa/jwst/jwst.rst Outdated Show resolved Hide resolved
docs/esa/jwst/jwst.rst Outdated Show resolved Hide resolved
docs/esa/jwst/jwst.rst Outdated Show resolved Hide resolved
docs/esa/jwst/jwst.rst Outdated Show resolved Hide resolved
@jespinosaar
Copy link
Contributor Author

Thanks for your feedback @eerovaher , @keflavich , @bsipocz ! everything is now solved so, if you agree, we can merge this PR.

@jespinosaar
Copy link
Contributor Author

Dear all, can we close this PR?

@bsipocz
Copy link
Member

bsipocz commented Jun 27, 2022

Dear all, can we close this PR?

I'm looking into this, I suppose vacation season means we're away from the keyboard from time to time (I certainly was away having some ⛰️ refill in the past few days) 😄

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't yet gone into any nitpicking details but first wonder whether you would have the capacity to remove the doctest-skip-all directive from the top to start running doctest on these code snippets?

After that removal you would need to add the .. doctest-remote-data: for each and every code block, but hopefully that is all, as the examples are very freshly made I suppose they will pass running them as tests.

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say that fixing the ... vs >>> issues are the only thing that needs to be fixed for merging, everything else is nitpicking, or thinking out loudly, or suggesting for future improvements.

I would love to have this module's narrative docs doctested (as said in the previous comment), but that can be done in a new follow-up PR. I think it's likely enough to change all the .. code-block:: python lines to .. doctest-remote-data::, and maybe some tweaking is needed for the outputs and formatting, but I don't expect much complications. Anyways, we can do this gradually, e.g. merging this PR and do opt out into testing in a new PR.

docs/esa/jwst/jwst.rst Outdated Show resolved Hide resolved
docs/esa/jwst/jwst.rst Outdated Show resolved Hide resolved
docs/esa/jwst/jwst.rst Outdated Show resolved Hide resolved
docs/esa/jwst/jwst.rst Outdated Show resolved Hide resolved
@jespinosaar
Copy link
Contributor Author

Dear all, can we close this PR?

I'm looking into this, I suppose vacation season means we're away from the keyboard from time to time (I certainly was away having some ⛰️ refill in the past few days) 😄

Dear @bsipocz , I am so sorry, I did not know you were away... Of course, do not worry if you are still on holiday, I will solve the issues in the meantime...

@jespinosaar
Copy link
Contributor Author

Ok, I think all the required changes have been applied.

@bsipocz
Copy link
Member

bsipocz commented Jun 28, 2022

I went ahead and fixed the remaining issues, the main ones:

  • some of the queries run into ValueError due to authentication issues. I skipped those for now, but some docs would be nice for it.
UNEXPECTED EXCEPTION: ValueError("Cannot retrieve products for observation jw01122001001_0210r_00001_nrs2: Error 403:\nPrivate file(s) requested: MAST token required for authentication.\n\nCannot process request: 'https://jwst.esac.esa.int/server/data' (req: Reqid: anonymous1656430293286, retrieval access: DIRECT, retrieval type: OBSERVATION, compression: null), for user: owner id: anonymous, roles: 0, session: C66DC70A30E7CCDDD6FE569302E2BFF9, ip: 73.109.49.174,  due to: Private file(s) requested: MAST token required for authentication.")
Traceback (most recent call last):

There are a lot of INFO messages coming from the module, I wonder whether those could be silenced, maybe by default. At the moment I've added them wherever they were issues.

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some examples have been temporarily skipped, otherwise, all of them pass the tests now.

@bsipocz bsipocz merged commit 26618d1 into astropy:main Jun 28, 2022
@bsipocz
Copy link
Member

bsipocz commented Jun 28, 2022

Thanks @jespinosaar!

@jespinosaar
Copy link
Contributor Author

I went ahead and fixed the remaining issues, the main ones:

* some of the queries run into ValueError due to authentication issues. I skipped those for now, but some docs would be nice for it.
UNEXPECTED EXCEPTION: ValueError("Cannot retrieve products for observation jw01122001001_0210r_00001_nrs2: Error 403:\nPrivate file(s) requested: MAST token required for authentication.\n\nCannot process request: 'https://jwst.esac.esa.int/server/data' (req: Reqid: anonymous1656430293286, retrieval access: DIRECT, retrieval type: OBSERVATION, compression: null), for user: owner id: anonymous, roles: 0, session: C66DC70A30E7CCDDD6FE569302E2BFF9, ip: 73.109.49.174,  due to: Private file(s) requested: MAST token required for authentication.")
Traceback (most recent call last):

There are a lot of INFO messages coming from the module, I wonder whether those could be silenced, maybe by default. At the moment I've added them wherever they were issues.

That is because we currently don't have data, there will be public data after the ERO release on July, 13th, and then I will update the examples with real download. Many thanks for looking into this!

@jespinosaar
Copy link
Contributor Author

Many thanks @bsipocz !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants