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

MolecularAtmosphere.ussa_1976() fix #334

Merged
merged 2 commits into from Jun 29, 2023
Merged

MolecularAtmosphere.ussa_1976() fix #334

merged 2 commits into from Jun 29, 2023

Conversation

nollety
Copy link
Contributor

@nollety nollety commented Jun 8, 2023

Description

Hotfix for an issue described in #332, that affects MolecularAtmosphere.ussa_1976().
This fix is not intended to remain very long; I expect the next atmosphere design (due for end of the month) to include a default (ckd/mono) absorption dataset that supports most spectral settings so that the default atmosphere constructor can be used across many (spectral and thermophysical) settings.

Changes

I have added a wavelength_range optional (defaults to 545-555 nm) parameter to the MolecularAtmosphere.ussa_1976 constructor that provide the information required for the constructor to automatically open the relevant absorption datasets. This makes the default constructor usable again:

atmosphere = MolecularAtmosphere.ussa_1976()

New usage

  • To instantiate a USSA 1976 molecular atmosphere:

    atmosphere = MolecularAtmosphere.ussa_1976()

    This atmosphere can be integrated in any experiment targetting wavelengths within [545, 555] nm

  • Outside of [545, 555] nm, specify the desired wavelength range, e.g.:

    from eradiate import unit_registry as ureg
    
    atmosphere = MolecularAtmosphere.ussa_1976(
      wavelength_range=[1580, 1630] * ureg.nm,
    )

Notes

Follow-up/related to: Experiments context used for initialization

Checklist

  • The code follows the relevant coding guidelines
  • The code generates no new warnings
  • The code is appropriately documented
  • The code is tested to prove its function
  • The feature branch is rebased on the current state of the main branch
  • I updated the change log if relevant
  • I give permission that the Eradiate project may redistribute my contributions under the terms of its license

@nollety nollety added bug 🐛 Something isn't working enhancement 🦾 New feature or request labels Jun 8, 2023
@nollety nollety force-pushed the hotfix-332 branch 2 times, most recently from b581e37 to 3dbade8 Compare June 8, 2023 13:19
@nollety nollety marked this pull request as ready for review June 8, 2023 13:19
@nollety nollety requested a review from leroyvn June 8, 2023 13:20
Copy link
Member

@leroyvn leroyvn left a comment

Choose a reason for hiding this comment

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

Hi @nollety, thanks for taking care of this. I made a few specific comments; in addition, I have a couple of general comments / questions:

  • Please briefly document the new syntax in this PR. It is referenced by the change log and will help users upgrade their codebase if they need.
  • Depending on the number of files, is it much more harmlful to xr.open_mfdataset() all the files? Doing so would considerably simplify the logic.

src/eradiate/radprops/_afgl1986.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/eradiate/scenes/atmosphere/_molecular_atmosphere.py Outdated Show resolved Hide resolved
src/eradiate/scenes/atmosphere/_molecular_atmosphere.py Outdated Show resolved Hide resolved
tests/02_eradiate/01_unit/radprops/test_us76_approx.py Outdated Show resolved Hide resolved
tests/02_eradiate/01_unit/radprops/test_us76_approx.py Outdated Show resolved Hide resolved
tests/02_eradiate/01_unit/radprops/test_us76_approx.py Outdated Show resolved Hide resolved
@nollety
Copy link
Contributor Author

nollety commented Jun 12, 2023

  • Depending on the number of files, is it much more harmlful to xr.open_mfdataset() all the files? Doing so would considerably simplify the logic.

xr.open_mfdataset() takes a string glob or a list of files to open as first parameter. That does not work here because we have a list of datasets (they are opened so that they can be sliced along the wavelength dimension).

This is also planned for an upgrade with thermoprops-next (support for customized atmospheric profiles).

Copy link
Member

@leroyvn leroyvn left a comment

Choose a reason for hiding this comment

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

Hi @nollety, I have a few more comments.

@nollety
Copy link
Contributor Author

nollety commented Jun 29, 2023

All tests passed except
tests/01_eradiate/02_system/test_basic.py::test_radiometric_accuracy[mono_single-1.0-constant-500000.0] which failed by a few decimals (see test output exerpt below).

E           AssertionError: 
E           Not equal to tolerance rtol=0.001, atol=0
E           
E           Mismatched elements: 1 / 10 (10%)
E           Max absolute difference: 0.00054938
E           Max relative difference: 0.00109875
E            x: array([0.499653, 0.500549, 0.500051, 0.499704, 0.500148, 0.499981,
E                  0.50008 , 0.499871, 0.499665, 0.49982 ], dtype=float32)
E            y: array([0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5])

@leroyvn
Copy link
Member

leroyvn commented Jun 29, 2023

All tests passed except tests/01_eradiate/02_system/test_basic.py::test_radiometric_accuracy[mono_single-1.0-constant-500000.0] which failed by a few decimals (see test output exerpt below).

E           AssertionError: 
E           Not equal to tolerance rtol=0.001, atol=0
E           
E           Mismatched elements: 1 / 10 (10%)
E           Max absolute difference: 0.00054938
E           Max relative difference: 0.00109875
E            x: array([0.499653, 0.500549, 0.500051, 0.499704, 0.500148, 0.499981,
E                  0.50008 , 0.499871, 0.499665, 0.49982 ], dtype=float32)
E            y: array([0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5])

This is unrelated with this PR. The test using the new illumination model barely passes the threshold and fails on several of my machines, but not always. I'll try and fix that now.

@nollety nollety marked this pull request as ready for review June 29, 2023 15:01
@leroyvn
Copy link
Member

leroyvn commented Jun 29, 2023

Thanks @nollety, looks like we're good to go 🚀

@leroyvn leroyvn merged commit 3e1b7c3 into main Jun 29, 2023
@nollety nollety deleted the hotfix-332 branch July 3, 2023 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working enhancement 🦾 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants