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

Consistency checks #341

Closed
wants to merge 11 commits into from
Closed

Conversation

FreddieWitherden
Copy link
Contributor

At the moment attempting to do something like:

props = Property(dimension=3, index_capacity=25, leaf_capacity=25)
idx = Index([], properties=props)

will throw an exception because the near minimum overlap factor is greater than the leaf/index capacities. Interestingly (and for reasons I can not understand) it does not throw an exception if [] is replaced by actual entries. It is only an issue for an initially empty index.

Either way, this PR adds some consistency checks so everything works as expected.

@hobu
Copy link
Member

hobu commented Dec 15, 2024

Looks like the code formatting nanny is failing on this.

@hobu hobu closed this Dec 15, 2024
@hobu hobu reopened this Dec 15, 2024
@FreddieWitherden
Copy link
Contributor Author

I just looked over the code changes and all of the formatting seems fine.

@hobu
Copy link
Member

hobu commented Dec 15, 2024

ruff-format is changing something, and the hook fails if of the nannies change anything. Maybe a false positive? 🤷

@FreddieWitherden
Copy link
Contributor Author

I think so?

Does the approach seem reasonable overall? I am unsure why it does not fail when we provide objects to bulk-insert. Maybe there is a check missing on the spatialindex side?

@hobu
Copy link
Member

hobu commented Dec 15, 2024

It is just a complaint about code formatting as far as I understand. Logic and layout of PR looks fine to me.

@FreddieWitherden
Copy link
Contributor Author

Okay, I think we are okay to merge.

@hobu
Copy link
Member

hobu commented Dec 16, 2024

Ruff is complaining about your quoting. Need to fix this or it will always complain

diff --git a/rtree/index.py b/rtree/index.py
index ef18743..92d51b8 100644
--- a/rtree/index.py
+++ b/rtree/index.py
@@ -1539,7 +1539,7 @@ class Property:
                 setattr(self, k, v)
 
         # Consistency checks
-        if 'near_minimum_overlap_factor' not in state:
+        if "near_minimum_overlap_factor" not in state:
             nmof = self.near_minimum_overlap_factor
             ilc = min(self.index_capacity, self.leaf_capacity)
             if nmof >= ilc:

@hobu
Copy link
Member

hobu commented Dec 16, 2024

did you mean to close this without merging?

@FreddieWitherden
Copy link
Contributor Author

Yes, now part of #342

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