Skip to content

i97 Insert observations grouped first by network#176

Open
QSparks wants to merge 15 commits intomasterfrom
i97
Open

i97 Insert observations grouped first by network#176
QSparks wants to merge 15 commits intomasterfrom
i97

Conversation

@QSparks
Copy link

@QSparks QSparks commented Jul 31, 2023

Added two functions to insert.py that will sort an arbitrary list of observations into a dictionary where <network_name> is the key. This way network-level logging is maintained even if calls to insert have observations from multiple networks.

resolves #97

@QSparks QSparks requested a review from jameshiebert July 31, 2023 17:40
Copy link
Contributor

@jameshiebert jameshiebert left a comment

Choose a reason for hiding this comment

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

The overall structure of the approach that you've taken looks great, but I have a couple of questions/comments that I think need to be addressed.

If you're able to write some unit tests that exercise these new functions, I think that would be helpful, because my read of the get_network() function is that it will not work (w.r.t. the caching) as written.

@QSparks QSparks requested a review from jameshiebert August 8, 2023 16:57
Copy link
Contributor

@jameshiebert jameshiebert left a comment

Choose a reason for hiding this comment

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

It's looking much better! Good work 👍

A few more comments that I think will further improve the code.

@QSparks QSparks requested a review from jameshiebert August 9, 2023 21:34
Copy link
Contributor

@jameshiebert jameshiebert left a comment

Choose a reason for hiding this comment

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

Looks much better! Last two comments and then I think we're done:

  1. I think log_helpers as a separate module is a good idea for these two functions. However, the name is a bit of a misnomer, since cached_function isn't really used in logging. How about db_helpers.py?
  2. I think that it's important for db_metrics to be returned (or yielded) per network. Different networks have different performance characteristics. I'd like to you swap the loops there and have each per-network iteration yield the db metrics results. You may need to adjust the calling code in process.py to make it work.

@QSparks QSparks requested a review from jameshiebert August 10, 2023 18:20
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.

Insert logging assumes homogenous networks

2 participants