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

Export inrangecount from NearestNeighbors.jl #17

Merged
merged 10 commits into from
Oct 22, 2022

Conversation

kahaaga
Copy link
Contributor

@kahaaga kahaaga commented Oct 22, 2022

Export inrangecount from NearestNeighbors.jl, as discussed in JuliaDynamics/ComplexityMeasures.jl#71 (comment) and in #16.

Is this approach ok? I'm not sure whether to export inrangecount directly, or override it with its own docstring here.

@Datseris
Copy link
Member

i's okay but also bump patch version!

@Datseris
Copy link
Member

Hm, actually it isn't. This should be defined in the API file with a "search strcture" terminology. Then, the file that extends the API based on NearestNeighbors.jl should provide the concrete implementation.

@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2022

Codecov Report

Base: 86.82% // Head: 85.86% // Decreases project coverage by -0.95% ⚠️

Coverage data is based on head (85533b5) compared to base (fa1ed57).
Patch coverage: 40.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #17      +/-   ##
==========================================
- Coverage   86.82%   85.86%   -0.96%     
==========================================
  Files           6        6              
  Lines         167      184      +17     
==========================================
+ Hits          145      158      +13     
- Misses         22       26       +4     
Impacted Files Coverage Δ
src/api.jl 74.35% <0.00%> (-5.06%) ⬇️
src/kdtree.jl 85.07% <100.00%> (+0.07%) ⬆️
src/bruteforce.jl 100.00% <0.00%> (ø)
src/Testing.jl 93.33% <0.00%> (+0.65%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kahaaga
Copy link
Contributor Author

kahaaga commented Oct 22, 2022

Hm, actually it isn't. This should be defined in the API file with a "search strcture" terminology

Defining e.g. struct WithinRangeCount; r; end in api.jl, you mean? And then in kdtree.jl define the following?

function Neighborhood.inrangecount(tree::KDTree, querypt, t::WithinRangeCount)
    ....
end

@kahaaga
Copy link
Contributor Author

kahaaga commented Oct 22, 2022

@Datseris Ok, I think I got what you meant, and updated the PR accordingly. Changes:

  • In api.jl: Added the WithinRangeCount search type, the inrangecount function.
  • In kdtree.jl: Dispatch on inrangecount(::KDTree, query, ::WithinRangeCount).
  • Added entries for WithinRangeCount and inrangecount in the documentation.
  • Added some tests.

@Datseris
Copy link
Member

That's pretty much what I've meant. However I removed the type because, by the definition of its name, inrangecount always uses the WithinRange type of search, so an additional struct is not necessary.

@kahaaga
Copy link
Contributor Author

kahaaga commented Oct 22, 2022

Nice! I'll update the open PRs in Entropies.jl to use the new version of Neighborhood.jl (I assume this is merged and tagged within not-too-long).

@Datseris Datseris merged commit 53be65e into JuliaNeighbors:master Oct 22, 2022
@kahaaga kahaaga deleted the inrangecount branch October 22, 2022 18:42
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.

3 participants