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 weights and axis parameters to vonmisesmle #14533

Merged

Conversation

FelipeCybis
Copy link
Contributor

@FelipeCybis FelipeCybis commented Mar 14, 2023

Add weights and axis parameters to vonmisesmle

This pull request is to address the lack of "weights" and "axis" parameters on the circstats function vonmisesmle.
The "axis" parameter that existed but was not actually being used allows for fast usage of 2D numpy arrays.
The "weights" parameter is very convenient since it is very common to have data for binned angles.

The appropriate tests were added to test_circstats.test_vonmisesmle in 4c6172f, but here is a minimum example using scipy's vonmises PDF:

>>> from astropy import stats
>>> import scipy
>>> x = np.linspace(0,2*np.pi,100)
>>> mu, kappa = stats.circstats.vonmisesmle(x, weights=scipy.stats.vonmises.pdf(x, loc = np.pi/2, kappa=4))
>>> mu, kappa
(1.56976133088732, 3.9514188541707314)

Or for 2D arrays:

>>> x = np.linspace(0,2*np.pi,100)
>>> xx = np.tile(x, [4,1])*u.rad
>>> vm = np.stack([
    scipy.stats.vonmises.pdf(x, loc = np.pi/2, kappa=4),
    scipy.stats.vonmises.pdf(x, loc = np.pi/2, kappa=1),
    scipy.stats.vonmises.pdf(x, loc = np.pi/4, kappa=4),
    scipy.stats.vonmises.pdf(x, loc = np.pi/8, kappa=8)])
>>> stats.circstats.vonmisesmle(xx, weights=vm, axis=1)
(<Quantity [1.56976133, 1.55292537, 0.77316812, 0.37759809] rad>,
 <Quantity [3.95141885, 0.98695686, 3.91289117, 7.95905254]>)

Fixes # There is not opened issue, since it is an enhancement.

@github-actions
Copy link

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@pllim pllim added this to the v5.3 milestone Mar 14, 2023
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to Astropy 👋 and congratulations on your first pull request! 🎉

A project member will respond to you as soon as possible; in the meantime, please have a look over the Checklist for Contributed Code and make sure you've addressed as many of the questions there as possible.

If you feel that this pull request has not been responded to in a timely manner, please send a message directly to the development mailing list. If the issue is urgent or sensitive in nature (e.g., a security vulnerability) please send an e-mail directly to the private e-mail feedback@astropy.org.

@pllim
Copy link
Member

pllim commented Mar 14, 2023

Thanks, you might also be interested in the discussions at #13749

@pllim pllim added the API change PRs and issues that change an existing API, possibly requiring a deprecation period label Mar 14, 2023
@FelipeCybis FelipeCybis force-pushed the add-weights-and-axis-vonmisesmle branch from e2552ea to ddff666 Compare March 14, 2023 23:23
@@ -0,0 +1,3 @@
This pull request is to address the lack of "weights" and "axis" parameters on the circstats function ``vonmisesmle``.
Copy link
Member

Choose a reason for hiding this comment

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

I would rephrase the change log text to the tone of what you want user to know about the changes when they read the release change log. They would not care it was from a pull request. Feel free to look at existing change log texts for examples. Thanks!

@FelipeCybis FelipeCybis force-pushed the add-weights-and-axis-vonmisesmle branch from ddff666 to 4c6172f Compare March 15, 2023 13:25
@FelipeCybis
Copy link
Contributor Author

Thanks for directing me to #13749. Indeed, could be interesting taking the best of both packages in the long run. Particularly I could certainly not work without weighing my arrays (hence the PR ;)). Also if you guys decide on adapting the kappa estimation using the scipy method it should be straight forward with the weights and axis params.

Also, tell me if I can still improve this PR in some way :)

Comment on lines 3 to 4
- The "axis" parameter allows fast computation using N-D numpy arrays.
- The "weights" parameter is very convenient when using data with binned angles.
Copy link
Member

Choose a reason for hiding this comment

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

Are these supposed to be bullet points? @saimn , does towncrier render bullets properly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I should have put a line between the paragraph and the bullet points. I don't know about towncrier, but if it handles .rst it should handle bullet points?
I can delete these two points all around. If the user knows how to use the weights and axis for the rest of the module, they will understand the idea of the change?
Up to you, I can either remove them or add the line between paragraph and bullet points

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, I finally deleted the bullet points as they were not adding much to the change log indeed.

@FelipeCybis FelipeCybis force-pushed the add-weights-and-axis-vonmisesmle branch from 4c6172f to 4e9886e Compare March 21, 2023 15:07
@pllim
Copy link
Member

pllim commented Apr 18, 2023

@FelipeCybis , sorry for the delay. Coverage is still complaining but the log is too old now, so I cannot see if the coverage upload failed or is it because of something else. Could you please rebase?

@larrybradley , are you able to review this before feature freeze?

Thanks!

Copy link
Member

@larrybradley larrybradley left a comment

Choose a reason for hiding this comment

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

Thanks, @FelipeCybis. LGTM, with one minor requested change.

astropy/stats/circstats.py Outdated Show resolved Hide resolved
Use ``_length`` to compute the generalized sample length
- This change will allow weighted vonmisesmle and the use of the
  ``axis`` parameter in the correct way

Add test to use axis and weights parameters in `vonmisesmle`

Update astropy/stats/circstats.py

Add @larrybradley requested change

Co-authored-by: Larry Bradley <larry.bradley@gmail.com>
@FelipeCybis FelipeCybis force-pushed the add-weights-and-axis-vonmisesmle branch from 6de305e to 47ab1c3 Compare April 18, 2023 16:10
@pllim
Copy link
Member

pllim commented Apr 18, 2023

pre-commit.ci autofix

data = np.array(
[
[
3.3699057,
Copy link
Member

Choose a reason for hiding this comment

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

Seems like black overdid it a little but this is what would make the precommit bot happy. 🤷

Copy link
Member

Choose a reason for hiding this comment

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

I really don't like this formatting. We could manually reformat and turn off black (fmt: off), but let's just get this in before the freeze.

Copy link
Member

@larrybradley larrybradley left a comment

Choose a reason for hiding this comment

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

Thanks, @FelipeCybis

@larrybradley larrybradley merged commit 4f3a54b into astropy:main Apr 18, 2023
25 checks passed
@pllim
Copy link
Member

pllim commented Apr 18, 2023

Thanks, all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change PRs and issues that change an existing API, possibly requiring a deprecation period stats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants