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

Add angle_to_dir to convert angles to nearest direction #853

Merged
merged 8 commits into from Sep 27, 2019

Conversation

ahuang11
Copy link
Contributor

@ahuang11 ahuang11 commented May 17, 2018

Also, minor enhancement to make parse_angle handle pd.Series and np.array. Fixes problem with missing data too.

metpy/calc/tools.py Outdated Show resolved Hide resolved
metpy/calc/tools.py Outdated Show resolved Hide resolved
@dopplershift dopplershift added this to the 0.9 milestone May 17, 2018
metpy/calc/tests/test_tools.py Outdated Show resolved Hide resolved
metpy/calc/tests/test_tools.py Outdated Show resolved Hide resolved
metpy/calc/tools.py Outdated Show resolved Hide resolved
metpy/calc/tools.py Outdated Show resolved Hide resolved
@dopplershift dopplershift added Type: Feature New functionality Area: Calc Pertains to calculations labels May 18, 2018
@ahuang11
Copy link
Contributor Author

Need to add feature to handle missing data, or else it crashes on nan.

metpy/calc/tools.py Outdated Show resolved Hide resolved
@jrleeman
Copy link
Contributor

jrleeman commented Jun 4, 2018

@ahuang11 Let us know if you get stuck anywhere on the CI failures. We have a known issue on 3.4, but will be dropping it shortly.

metpy/calc/tests/test_tools.py Outdated Show resolved Hide resolved
metpy/calc/tools.py Show resolved Hide resolved
metpy/calc/tools.py Outdated Show resolved Hide resolved
@ahuang11
Copy link
Contributor Author

ahuang11 commented Jun 8, 2018

Okay I'm not too sure why those tests are failing; one has to do with Python 3.4, but the other two I'm confused.

@dopplershift dopplershift modified the milestones: 0.9, 0.10 Aug 17, 2018
@jrleeman jrleeman modified the milestones: 0.10, 0.11 Dec 17, 2018
@ahuang11
Copy link
Contributor Author

Not exactly sure why the tests are failing.

@dopplershift
Copy link
Member

This is the relevant line from the logs:

ERROR - The following functions are missing from the override file metpy.calc.rst: angle_to_dir

What it means is that you need to add angle_to_dir under the appropriate heading in _templates/overrides/metpy.calc.rst so that the function shows up in the categorized reference guide for the calculations.

@ahuang11
Copy link
Contributor Author

I think this is ready to be reviewed

@CLAassistant
Copy link

CLAassistant commented Jun 28, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@zbruick zbruick left a comment

Choose a reason for hiding this comment

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

I think we're looking to get this PR into 0.11, so if you're still interested in working on this, I have a few suggestions to think about. It will also need to be rebased on master and conflicts resolved, as quite a bit has been merged in since this was originally written. Let us know if you have any questions and thanks for your work!

examples/calculations/Angle_to_Direction.py Outdated Show resolved Hide resolved
metpy/calc/tests/test_calc_tools.py Show resolved Hide resolved
metpy/calc/tests/test_calc_tools.py Outdated Show resolved Hide resolved
metpy/calc/tools.py Outdated Show resolved Hide resolved
metpy/calc/tests/test_calc_tools.py Outdated Show resolved Hide resolved
Copy link
Contributor

@zbruick zbruick left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! Just a couple things to fix to get CI to pass. Also it doesn't looked like you've signed the CLA, which would need to be done before merging.

docs/_templates/overrides/metpy.calc.rst Outdated Show resolved Hide resolved
metpy/calc/tests/test_calc_tools.py Outdated Show resolved Hide resolved
@dopplershift
Copy link
Member

I have ahuang11 in the CLA list. GitHub is showing some commits with a different icon, which I'm guessing it related to the CLA issue. Unfortunately, looking at the commit log here on my system, I cannot for the life of me find any difference. So we can just merge over CLA if necessary.

zbruick
zbruick previously approved these changes Aug 29, 2019
Copy link
Contributor

@zbruick zbruick left a comment

Choose a reason for hiding this comment

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

Looks good now. I think we just need a rebase to squash down a bunch of these commits and then we'll be good to go.

Optimized parse_angle() for long list of directional strings.

Move "from operator import itemgetter" up one line.

Revise how DIR_DICT is initialized; will use BASE_DEGREE_MULTIPLIER more in another PR.

Remove print in test_tools

Add back extra line...

Handle np.array and pd.Series for parse_angle() and add new angle_to_dir()

Fix example heading section

Add full=True example

Imperative Mood and newline between import group

Update templates doc

Fix CI

Add back year
Copy link
Contributor

@zbruick zbruick left a comment

Choose a reason for hiding this comment

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

So the rebase went ok, except that the unused issue crept up again. Just remove this import and we should be good to go.

examples/calculations/Angle_to_Direction.py Outdated Show resolved Hide resolved
Copy link
Contributor

@zbruick zbruick left a comment

Choose a reason for hiding this comment

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

Just need to switch to the new function name in the example

examples/calculations/Angle_to_Direction.py Outdated Show resolved Hide resolved
examples/calculations/Angle_to_Direction.py Outdated Show resolved Hide resolved
examples/calculations/Angle_to_Direction.py Outdated Show resolved Hide resolved
Copy link
Contributor

@zbruick zbruick left a comment

Choose a reason for hiding this comment

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

Thanks for making that last change. All the checks are now passing so we're good to go, but there's just a small copyright conflict. Can you accept the one that lists all of the years this file has been updated?

zbruick
zbruick previously approved these changes Sep 18, 2019
Copy link
Contributor

@zbruick zbruick left a comment

Choose a reason for hiding this comment

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

The CI errors are due to xarray releasing v0.13, which is being fixed in #1144.

Copy link
Contributor

@zbruick zbruick left a comment

Choose a reason for hiding this comment

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

Codecov isn't reporting despite being sent on all coverage runs on Travis. It passed before so I'm not concerned about it. @dopplershift, can you merge over it?

@zbruick zbruick merged commit f377a48 into Unidata:master Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Calc Pertains to calculations Type: Feature New functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants