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

Added units to the docstrings #68

Merged
merged 16 commits into from
Feb 15, 2024
Merged

Conversation

RashmikaReddy
Copy link

#53

@RashmikaReddy RashmikaReddy marked this pull request as ready for review February 2, 2024 03:32
@RashmikaReddy RashmikaReddy requested a review from uwcdc February 2, 2024 03:32
@uwcdc uwcdc changed the base branch from dev to docstrings February 5, 2024 22:28
Copy link
Member

@uwcdc uwcdc left a comment

Choose a reason for hiding this comment

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

Please check the consistency in abbreviations and unit justification spacings throughout. It also looks like you missed some units that you defined in other places throughout the code. Be sure to Preview each file that you changed to make sure that it renders properly in the JupyterBook.

src/caustics/cosmology.py Outdated Show resolved Hide resolved
src/caustics/cosmology.py Outdated Show resolved Hide resolved
src/caustics/cosmology.py Outdated Show resolved Hide resolved
src/caustics/lenses/base.py Outdated Show resolved Hide resolved
src/caustics/lenses/base.py Outdated Show resolved Hide resolved
src/caustics/lenses/epl.py Outdated Show resolved Hide resolved
src/caustics/lenses/multiplane.py Outdated Show resolved Hide resolved
src/caustics/lenses/multiplane.py Outdated Show resolved Hide resolved
src/caustics/lenses/multiplane.py Outdated Show resolved Hide resolved
src/caustics/lenses/sie.py Outdated Show resolved Hide resolved
@RashmikaReddy
Copy link
Author

RashmikaReddy commented Feb 12, 2024

Please check the consistency in abbreviations and unit justification spacings throughout. It also looks like you missed some units that you defined in other places throughout the code. Be sure to Preview each file that you changed to make sure that it renders properly in the JupyterBook.

Maintained consistent units and spacing throught to the best of my knowledge. However, when renderred in jupyterbook, it shows how it is written in the code itself - *Unit: arcsec*.

Copy link
Member

@uwcdc uwcdc left a comment

Choose a reason for hiding this comment

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

Review the changes, but overall a huge improvement.

src/caustics/lenses/epl.py Outdated Show resolved Hide resolved
src/caustics/lenses/nfw.py Outdated Show resolved Hide resolved
src/caustics/lenses/pseudo_jaffe.py Outdated Show resolved Hide resolved
src/caustics/lenses/pseudo_jaffe.py Outdated Show resolved Hide resolved
src/caustics/lenses/pseudo_jaffe.py Outdated Show resolved Hide resolved
src/caustics/lenses/sie.py Outdated Show resolved Hide resolved
Comment on lines 766 to 776

x_component: Tensor
Deflection Angle

*Unit: arcsec*

y_component: Tensor
Deflection Angle

*Unit: arcsec*

Copy link
Member

Choose a reason for hiding this comment

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

This should be right under Returns

Copy link
Member

Choose a reason for hiding this comment

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

Done.

Comment on lines 826 to 836

x_component: Tensor
Deflection Angle

*Unit: arcsec*

y_component: Tensor
Deflection Angle

*Unit: arcsec*

Copy link
Member

Choose a reason for hiding this comment

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

This should be under Returns

Copy link
Member

Choose a reason for hiding this comment

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

Done.

Comment on lines 342 to 352

x_component: Tensor
Deflection Angle

*Unit: radians*

y_component: Tensor
Deflection Angle

*Unit: radians*

Copy link
Member

Choose a reason for hiding this comment

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

This should be under Returns

Copy link
Member

Choose a reason for hiding this comment

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

Done

Copy link
Member

@lsetiawan lsetiawan left a comment

Choose a reason for hiding this comment

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

There are probably things that I missed since it's so huge, but we'll just continue working on it in future PRs. Thanks again @RashmikaReddy and @uwcdc for working on this. This is ready to merge after the above issues are fixed.

@lsetiawan lsetiawan merged commit b38dd83 into uw-ssec:docstrings Feb 15, 2024
1 of 2 checks passed
lsetiawan pushed a commit that referenced this pull request Feb 15, 2024
* Added units to the docstrings

* style: pre-commit fixes

* Adding another file

* Made changes to the docstrings

* style: pre-commit fixes

* Made the changes, fixed indentation

* style: pre-commit fixes

* Added spacing, fixed indentation,  added unitless

* Made changes and adjusted the spacing

* style: pre-commit fixes

* docs: Updated units and formatting including cosmology, lenses, and sources

* fix: Removed tuple and moved returns

* fix: Remove tuple from return and moved separated returns

* style: pre-commit fixes

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: uwcdc <[email protected]>
lsetiawan pushed a commit to lsetiawan/caustics that referenced this pull request Feb 15, 2024
* Added units to the docstrings

* style: pre-commit fixes

* Adding another file

* Made changes to the docstrings

* style: pre-commit fixes

* Made the changes, fixed indentation

* style: pre-commit fixes

* Added spacing, fixed indentation,  added unitless

* Made changes and adjusted the spacing

* style: pre-commit fixes

* docs: Updated units and formatting including cosmology, lenses, and sources

* fix: Removed tuple and moved returns

* fix: Remove tuple from return and moved separated returns

* style: pre-commit fixes

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: uwcdc <[email protected]>
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.

3 participants