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.

Sign up for GitHub

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

Jump to bottom

Units bug fix for _get_bound_pressure_height #1943

Merged
merged 2 commits into from Jun 29, 2021
Merged

Units bug fix for _get_bound_pressure_height #1943

merged 2 commits into from Jun 29, 2021

Conversation

23ccozad
Copy link
Contributor

It appears we were removing the units and only using the magnitude of bound, height, and pressure when interpolating pressure at a given height in _get_bound_pressure_height(), which is used in get_layer(). Therefore, when the height profile and bottom/top bound were in different units, the function failed miserably. Keeping those units in place resolves the issue.

Checklist

@23ccozad 23ccozad marked this pull request as ready for review June 28, 2021 19:51
@23ccozad 23ccozad added Area: Calc Pertains to calculations Type: Bug Something is not working like it should labels Jun 28, 2021
@23ccozad 23ccozad added this to the 1.1.0 milestone Jun 28, 2021
dopplershift
dopplershift requested changes Jun 28, 2021
View reviewed changes
Copy link
Member

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

This change looks good. Can you add a test based on the failing case in #1938?

@23ccozad
Copy link
Contributor Author

Can you add a test based on the failing case in #1938?

@dopplershift Issue #1938 uses get_layer(), which is in src/metpy/calc/tools.py, but I don't see a corresponding test file called tests/calc/test_tools.py. Should I find another function that calls get_layer() and write the test in the corresponding existing file, or create a new file called test_tools.py and write the test there?

@dopplershift
Copy link
Member

They're in tests/calc/test_calc_tools.py. I know there's a reason we named it that way, but don't have it off the top of my head.

dopplershift
dopplershift reviewed Jun 29, 2021
View reviewed changes
tests/calc/test_calc_tools.py Outdated
bottom=units.Quantity(1, 'km'),
depth=units.Quantity(5, 'km'),
height=height)
assert_array_almost_equal(pres_subset[1], units.Quantity(900, 'hPa'), 2)
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to just check that the entire pres_subset and temp_subset arrays have the expected values, rather than checking single elements?

@dopplershift dopplershift merged commit 05b8c0c into Unidata:main Jun 29, 2021
@23ccozad 23ccozad deleted the get_layer_heights_bug branch July 16, 2021 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dopplershift dopplershift dopplershift approved these changes

@dcamron dcamron Awaiting requested review from dcamron

Assignees
No one assigned
Labels
Area: Calc Pertains to calculations Type: Bug Something is not working like it should
Projects
None yet
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.

get_layer heights unit issue
2 participants
@23ccozad @dopplershift