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

Bandwidth selection in type_density and type_ridge #291

Merged
merged 9 commits into from
Jan 27, 2025

Conversation

zeileis
Copy link
Collaborator

@zeileis zeileis commented Jan 27, 2025

Grant @grantmcdermott, this is a follow-up to #270 where I did the following in three separate commits:

  • Made joint.bw = "owm" the default in type_density() and enhanced the documentation. In particular, I added a new section explaining the practical considerations for choosing the different joint.bw alternatives. I wrote this mostly so that we do not forget about it but feel free to shorten it (or remove it entirely) if you feel that this is too detailed.
  • Update the type_ridge() documentation to the changes in the type_density() documentation.
  • Use joint.bw = "mean" instead of "owm". I find the latter too opaque and it over-emphasizes the weighting. I find "mean" much more straightforward and the weighting is natural (to me) and well-explained in the documentation. If you feel strongly about not using "mean", feel free to revert this commit.

If you keep the last commit, you way want to further adjust the tests. I have changed their code so that they run successfully but kept "owm" in the annotations so that the snapshots don't need updating, yet.

Closes #290

@zeileis
Copy link
Collaborator Author

zeileis commented Jan 27, 2025

Sorry, I forgot to check the density default checks more carefully hence the checks failed. Nevertheless I'll first wait for your feedback or changes before proceeding with updating the tests.

@grantmcdermott
Copy link
Owner

This is fantastic. Thanks @zeileis!

I agree with all of these changes. I'll quickly update the test snapshots on my side and push them so that the CI checks are passing.

I can also think of one or two additions that we can add as part of this PR too. (E.g., I need to update the NEWS file.) I'll try to add those tonight.

@grantmcdermott grantmcdermott merged commit f641635 into main Jan 27, 2025
3 checks passed
@zeileis zeileis deleted the density-ridge branch January 27, 2025 12:08
@zeileis
Copy link
Collaborator Author

zeileis commented Jan 27, 2025

Excellent, thanks for wrapping all of this up! 👌 I cleaned up the corresponding branches now.

I noticed that there is an occurrence of @zeleis as opposed to @zeileis in the NEWS. Can I just fix such trivial typos directly in the main branch or should I go through a proper PR?

@vincentarelbundock
Copy link
Collaborator

FWIW, I've pushed trivial things like this in the past and Grant has not gotten angry at me yet :)

@zeileis
Copy link
Collaborator Author

zeileis commented Jan 27, 2025

Thanks, fixed now!

@grantmcdermott
Copy link
Owner

FWIW, I've pushed trivial things like this in the past and Grant has not gotten angry at me yet :)

Def feel free to fix trivial issues directly (and apologies for the typo).

@zeileis
Copy link
Collaborator Author

zeileis commented Jan 27, 2025

Will do - no worries about the typo!

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.

Switch to type_density(joint.bw = "mean") as default
3 participants