- 
                Notifications
    You must be signed in to change notification settings 
- Fork 29
feat(python/sedonadb): Expose random_geometry as a Python function #97
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
base: main
Are you sure you want to change the base?
Conversation
| This reminds me that when I was writing up the pytest-benchmarking code, I realized it would be nice to have a random geometry rust function (instead of a table provider) that returns a single column, so that we could create multiple unique random geometry columns in a single query to use with functions that take two geometry inputs (e.g predicates). I can't think of many other use cases tho, so it doesn't seem very high priority. | 
381126b    to
    7354cbf      
    Compare
  
    | width = bounds[2] - bounds[0] | ||
| height = bounds[3] - bounds[1] | ||
| if size_min > width or size_min > height: | ||
| raise ValueError("size > height / 2 or width / 2 of bounds") | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message does not match the check. The error message talks about halfs.
| empty_rate: float = 0.0, | ||
| null_rate: float = 0.0, | ||
| seed: Optional[int] = None, | ||
| ) -> "sedonadb.dataframe.DataFrame": | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Missing docstring
| f"Expected bounds as [xmin, ymin, xmax, ymax] but got {bounds}" | ||
| ) | ||
|  | ||
| width = bounds[2] - bounds[0] | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be checks that xmin is smaller than xmax ?
Same for the y bounds below.
| Thank you for the review! I will try to circle back to this in the next few days. One of the things I discovered here is that our random geometry generator can panic for some parameter combinations, which needed fixing at a lower level 😬 | 
We've been using the SQL table function
sd_random_geometry()quite a bit for testing and benchmarking and I think this may make those types of simulations much nicer to write.This still needs a bit of testing/moving of the parameter validation to Rust but it's not essential for release. I'll circle back after working on some more releasy-type tasks.