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

Fix find_maxima call in find_minima function #475

Merged

Conversation

Deuchnord
Copy link
Contributor

Pass epsilonand num parameters from find_minima to find_maxima.

I'd like to take advantage of this PR to add documentation about these parameters and the rough_period attribute set to the function, since they seem very important in the calculation. Personally, I've always failed to really understand what their purpose is. Can you explain these, so I can update the docs?

Pass epsilon and num parameters from find_minima to find_maxima.
@brandon-rhodes
Copy link
Member

I would prefer to update docs in a separate PR, to keep the history simpler, so I will be happy to go ahead and merge this one as-is! Thanks for pointing out this bug and contributing a fix.

Instead of my writing up a description here in this PR's comments about what those parameters mean, and then your having to repeat it as documentation, it might make more sense for me to just add some documentation to their docstrings directly. I was planning on writing some documentation this week (as there are a few other documentation-related issues open), so I could these parameters too.

Currently the only place I explain their meaning is in this talk, if you wanted to go watch it or review its slides:

https://rhodesmill.org/brandon/talks/#fiery-reentry

@Deuchnord
Copy link
Contributor Author

OK, then I put this PR as ready for review :)

@Deuchnord Deuchnord marked this pull request as ready for review November 9, 2020 12:40
@brandon-rhodes
Copy link
Member

Are there supposed to be CI checks that run on this repository? Did they get turned off as part of that transition that Travis CI is making? I'd better to take a look this afternoon.

@brandon-rhodes brandon-rhodes merged commit 5ecc513 into skyfielders:master Nov 9, 2020
@Deuchnord Deuchnord deleted the fix-minima-function-param branch November 9, 2020 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants