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

WIP: Reflectance concepts #329

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jianyangli
Copy link
Contributor

This PR is WIP and for discussion related to #328

* Rename `reflectance` to `dimensionless_albedo`
* Use disk-integrated albedo (product of geometric albedo and disk-integrated phase function) rather than disk-average bidirectional reflectance
* Add conversion between albedo and cross-section with a given total flux/magnitude
* Updated docstrings and doctests
* Add optional parameters `rh` and `delta` for future implementation.
* Update test data with the new albedo definition (albedo = reflectance * pi)
* Add tests for `albedo_unit`
* Add tests for convertion between albedo and size
* Expand tests to cover the conversion in both directions for all cases
* Replace np.isclose and np.allclose by u.isclose or u.allclose
Including all tests and remote tests.
@jianyangli jianyangli requested a review from mkelley May 3, 2022 14:25
@jianyangli jianyangli self-assigned this May 3, 2022
@jianyangli jianyangli mentioned this pull request May 3, 2022
5 tasks
@jianyangli jianyangli linked an issue May 4, 2022 that may be closed by this pull request
5 tasks
Copy link
Member

@mkelley mkelley left a comment

Choose a reason for hiding this comment

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

Is the PDF supposed to be included with the source code?

Please add an item to the CHANGELOG and note the API change (reflectance was replaced).

'projected_size',
]


Copy link
Member

Choose a reason for hiding this comment

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

I've recently learned that, in general, __all__ should be before import statements. https://peps.python.org/pep-0008/#module-level-dunder-names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strange. When I put __all__ before the import statements, pycodestyle check returns errors:

(dev) jyli@adeona code % pycodestyle sbpy/units/core.py
/Users/jyli/.env/dev/lib/python3.8/site-packages/pycodestyle.py:113: FutureWarning: Possible nested set at position 1
EXTRANEOUS_WHITESPACE_REGEX = re.compile(r'[[({] | []}),;:]')
sbpy/units/core.py:29:1: E402 module level import not at top of file
sbpy/units/core.py:30:1: E402 module level import not at top of file
sbpy/units/core.py:31:1: E402 module level import not at top of file
sbpy/units/core.py:32:1: E402 module level import not at top of file
sbpy/units/core.py:33:1: E402 module level import not at top of file
sbpy/units/core.py:34:1: E402 module level import not at top of file
sbpy/units/core.py:37:1: E402 module level import not at top of file
sbpy/units/core.py:38:1: E402 module level import not at top of file
sbpy/units/core.py:39:1: E402 module level import not at top of file

I'm confused. Maybe I'll just do # noqa: E402?

Copy link
Member

Choose a reason for hiding this comment

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

Well that's annoying. PEP8 has conflicting advice here. https://peps.python.org/pep-0008/#imports Then leave it as is. I'd rather not salt our code with noqa comments.

>>> xsec = np.pi * radius**2
>>> alb = xsec.to(albedo_unit, dimensionless_albedo('V', flux=mag))
>>> print('{0:.4f}'.format(alb))
0.0900 albedo
"""
Copy link
Member

Choose a reason for hiding this comment

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

Do you have a good reference for more information on albedo and reflectance concepts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I will need to include references when we have a decision on the best approach.

@jianyangli
Copy link
Contributor Author

Is the PDF supposed to be included with the source code?

What PDF?

@mkelley
Copy link
Member

mkelley commented May 14, 2022

sbpy/units/tests/data/hi05070405_9000036-avg-spec_ref.pdf It seems this file was already there. I noticed it because it was updated in the PR.

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

Successfully merging this pull request may close these issues.

Reflectance concepts
2 participants