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

kstar computation is mandatory when calling PAk #120

Merged
merged 6 commits into from
Mar 17, 2024

Conversation

diegodoimo
Copy link
Collaborator

@diegodoimo diegodoimo commented Mar 12, 2024

I made the kstar computation mandatory in PAk. I believe that not doing so results in wrong outputs when kstar has been set before since the current PAk implementation does not recompute kstar if this attribute is already set. For instance, if we call "compute_density_kNN() ", then we are forced to call "compute_kstar()" before calling compute_density_PAk() to make sure that we do not use a constant k value in PAk. By definition PAk requires the computation of kstar, this is its main job and purpose. Not doing so, can produce a weird hybrid algorithm in which the free energy is optimized over a set of ks that do not belong to the philosophy of the algorithm. For this reason, I suggest making the computation of kstar mandatory. This is a cheap part computationally speaking, and the overhead for the (rather rare) cases in which the user calls explicitly the "compue_kstar" method before pak are negligible.

@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 81.32%. Comparing base (f073bf4) to head (d94654a).
Report is 4 commits behind head on main.

Files Patch % Lines
dadapy/density_estimation.py 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #120      +/-   ##
==========================================
- Coverage   81.61%   81.32%   -0.29%     
==========================================
  Files          20       20              
  Lines        3285     3299      +14     
==========================================
+ Hits         2681     2683       +2     
- Misses        604      616      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AldoGl
Copy link
Collaborator

AldoGl commented Mar 12, 2024

I would be more in favour of raising a warning if all ks are constant. I believe one of the nice features of DADApy is the possibility of "mixing and matching" different algorithms. For instance, one might want to assess the performance of the PAk estimator with a specific choice of k, say for a constant k ranging from 10 to 1000. That said, since I am not a big user of these algorithms I would leave the choice to you, and perhaps to @charliematteo

@alexdepremia
Copy link
Collaborator

Indeed, I agree with Diego, PAk has no scope without first computing k-star. However, I'm not convinced about making mandatory the call to compute k-star. For instance, I'm working on an alternative way to compute k-star and I will like to introduce that in DADApy soon. I would go for Guido's suggestion of maintaining the flexibility of the code.

@charliematteo
Copy link
Collaborator

I also agree with @AldoGl to just raise a warning but leaving it more flexible for the users

@diegodoimo diegodoimo merged commit cd3391d into main Mar 17, 2024
8 of 9 checks passed
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.

5 participants