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

correct doc typos #231

Merged
merged 8 commits into from
Apr 12, 2023
Merged

correct doc typos #231

merged 8 commits into from
Apr 12, 2023

Conversation

marcobortolami
Copy link
Contributor

I went through the documentation in docs/source/ and found some typos, uncorrectly linked API functions, ... . I created this PR to fix them.

@marcobortolami
Copy link
Contributor Author

marcobortolami commented Apr 10, 2023

Few additional comments:

  1. In my opinion, this picture that is used here should be clarified a little bit more, e.g. by using the notation in the text just above it and by adding other vectors.
  2. Some of the links in README.md are not working because the corresponding sections do not exist, e.g. Prerequisites or Installation. These sections should be created.
  3. The links to the API in the bulleted list here are still not working. Should they be changed, e.g., from :func:`.HwpSys.set_parameters` to :func:`.hwp_sys.HwpSys.set_parameters` ?

Here a checklist to keep track of this:

  • Fix point 1
  • Fix point 2
  • Fix point 3

@marcobortolami
Copy link
Contributor Author

I tried to change a bit the picture of point 1 (by hand, I will improve it as soon as I understand better this point). I am certainly missing something, as the polarization angle in the figure should be 180°. Do you have any comment?
polarization-direction

@ziotom78
Copy link
Member

Thanks! I did some work on this PR to fix the issues you mentioned:

  1. In my opinion, this picture that is used here should be clarified a little bit more, e.g. by using the notation in the text just above it and by adding other vectors.

Right, the new image should be clearer:

image

The new text clarifies that the vector e_x has been drawn twice because the one at the top represents the polarization direction in the detector's reference frame, and it's what gets rotated to p.

2. Some of the links in [README.md](https://github.com/litebird/litebird_sim/blob/aca5c5e373b17085f1720752afe65140c8899966/README.md) are not working because the corresponding sections do not exist, e.g. Prerequisites or Installation. These sections should be created.

Right, they were generated automatically for some ancient version of the file, but they were never updated. Fixed.

3. The links to the API in the bulleted list [here](https://litebird-sim.readthedocs.io/en/master/external/hwp_sys.html) are still not working. Should they be changed, e.g., from `` :func:`.HwpSys.set_parameters`  `` to `` :func:`.hwp_sys.HwpSys.set_parameters`  ``?

Yes, but that's not enough, as :func: should be changed to :meth:, as they are methods of the class HwpSys. Fixed.

If you agree with my changes, we can merge this PR.

@marcobortolami
Copy link
Contributor Author

Thank you, Maurizio.

Right, the new image should be clearer:

Yes, it is! One last comment: the North direction is along a meridian, not a parallel, right? If so, I can change the word and close the PR.

Right, they were generated automatically for some ancient version of the file, but they were never updated. Fixed.

Thanks, I also removed the last broken "Acknowledgements" link.

Yes, but that's not enough, as :func: should be changed to :meth:, as they are methods of the class HwpSys. Fixed.

Thanks.

@marcobortolami marcobortolami merged commit 1f985e2 into master Apr 12, 2023
@marcobortolami marcobortolami deleted the correct_doc_typos branch April 12, 2023 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants