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 full output support in diptest #29

Merged
merged 5 commits into from
Nov 2, 2023
Merged

Conversation

prokolyvakis
Copy link
Contributor

What?

  1. The diptest does not currently support a full_output mode.
  2. The interpolation code in diptest reduces code readability.

Why?

There are many works in the literature, e.g., [1], that require both bootstraping and also get the full output since they need access for instance to the mode intervals. The current implementation does not support it, necessitating the need for two distinct calls.

  1. Maurus, S., & Plant, C. (2016, August). Skinny-dip: clustering in a sea of noise. In Proceedings of the 22nd ACM SIGKDD international conference on Knowledge discovery and data mining (pp. 1055-1064).

How?

  • The diptest function was extended to support an optional full_output input argument (disabled by default).
  • The interpolation code was refactored and now is part of the Consts class as a classmethod .

Further Architectural Changes

The diptest 'if else' logic was rewritten to ease readability. The previous version had a lot of similar code segments, i.e., diptest_pval_mt and diptest_pval with small argument differences.

Testing?

The implementation has been tested locally in a Darwin platform and all the unittests pass with success.

Varia

It's been a long time, I hope you are fine @RUrlus :)

@RUrlus RUrlus merged commit ad4c301 into RUrlus:stable Nov 2, 2023
24 checks passed
@RUrlus
Copy link
Owner

RUrlus commented Nov 2, 2023

@prokolyvakis it has been a while. I am good, I hope you're doing well as well:)

Thanks for the contribution! There we some minor typos in the threaded path which were apparently also not tested so I added that.

@RUrlus
Copy link
Owner

RUrlus commented Nov 2, 2023

I am generating new wheels which you can use when #30 has been merged

@prokolyvakis
Copy link
Contributor Author

Thanks @RUrlus! :)

@RUrlus
Copy link
Owner

RUrlus commented Nov 2, 2023

@prokolyvakis Are you working on reimplementing the above paper? I just skimmed it and I think it would be a cool addition to the package

@prokolyvakis
Copy link
Contributor Author

@RUrlus Yes, I have a draft implementation of it in a private repo (that's the reason I opened this PR). I can make it public and if you want I can make you a contributor! I don't believe that it would be hosted here, there could be proposed many alternatives of this method and it could overload the purpose of this repo!

@prokolyvakis prokolyvakis deleted the feature branch November 3, 2023 09:55
@RUrlus
Copy link
Owner

RUrlus commented Nov 3, 2023

@prokolyvakis makes sense! No need to make it public on my behalf, would be cool if you can give me a ping if/when you have made the repo public out of curiosity.

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.

None yet

2 participants