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

Get rid of boundary edges in the database #28

Merged
merged 5 commits into from
Sep 4, 2024
Merged

Conversation

dkhofer
Copy link
Collaborator

@dkhofer dkhofer commented Aug 29, 2024

No description provided.

@@ -360,6 +360,7 @@ fn main() {
sample,
}) => {
let mut conn = get_connection(db);
conn.execute("PRAGMA cache_size=50000;", []).unwrap();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This dropped the benchmark timing from 11s to 8.5-9s

@dkhofer dkhofer requested a review from Chris7 August 29, 2024 19:31
"INSERT OR IGNORE INTO block_group_edges (block_group_id, edge_id) VALUES {0};",
formatted_rows_to_insert
);
let _ = conn.execute(&insert_statement, ());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we even need a nassignment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rust compiler gives a warning if I don't have it, because execute may return an Err value. It seems like the shortest way of saying "yes, I actually mean to ignore the return value".

}

#[allow(clippy::len_zero)]
if block_boundaries.len() > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guessing thi swants you to use .empty() instead of len?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but I just tried again and it worked. I think I was in a rush and tried is_empty instead of !is_empty, which caused tests to break. I think at that point I had been fixing compiler and formatting errors for a while and just got fed up with it. Thanks for asking about this, it's fixed now

for (into, out_of) in sorted_sequence_edges.clone().into_iter().tuple_windows() {
let start = into.target_coordinate;
let end = out_of.source_coordinate;
for (start, end) in block_boundaries.clone().into_iter().tuple_windows() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is nice to know we have, i was just doing the other way for zipping lists

@dkhofer dkhofer merged commit 0e34168 into main Sep 4, 2024
1 check passed
@Chris7 Chris7 deleted the remove-boundary-edges branch September 24, 2024 16:31
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