Missing DegreeGroupedPermutations and performance tuning#84
Missing DegreeGroupedPermutations and performance tuning#84lagillenwater wants to merge 11 commits into
Conversation
…ulting to neo4j.het.io
… local nginx to serve them
…d w/data + convenience scripts
d33bs
left a comment
d33bs
left a comment
There was a problem hiding this comment.
This LGTM generally. The safety added for non-existent data looked sensible and the resource increases looked like they'd be useful.
@falquaddoomi - could we ask you to also take a look at this? @lagillenwater built upon the work you had started on this project.
@dhimmel - as a background, a group led by Casey and others is exploring how to increase the performance of data extractions from this project in order to address research questions. The changes here were started by @falquaddoomi and continued by @lagillenwater in order to see improvements when running a local instance of the database (where remote calls were otherwise not fast enough). Would you also have any thoughts about these changes or how we could reach a reasonable merge? Please don't hesitate to let us know if you have any thoughts and thank you in advance for your help.
| f'source_degree={source_degree}, target_degree={target_degree}. ' | ||
| f'Returning PathCount with null p-value.' | ||
| ) | ||
| dgp_record = None |
There was a problem hiding this comment.
Would it make sense to try and expand the tests for this case? I'm unsure how unusual or difficult this might be - so this could be an unreasonable question.
| if adj_p_value is not None: | ||
| metapath_score = -math.log10(adj_p_value) | ||
| else: | ||
| metapath_score = None |
There was a problem hiding this comment.
Consider adding a comment about when the adj_p_value might be None to help explain why we need this branching.
| try: | ||
| if instance.dgp is not None: | ||
| instance.dgp.reversed = reversed_ | ||
| has_dgp = True | ||
| except (DegreeGroupedPermutation.DoesNotExist, AttributeError): | ||
| pass |
There was a problem hiding this comment.
Consider adding some comments here about why we wouldn't have dgp.
There was a problem hiding this comment.
improvements when running a local instance of the database (where remote calls were otherwise not fast enough). Would you also have any thoughts about these changes or how we could reach a reasonable merge?
Thanks @d33bs for the context. I don't fully understand what this PR is trying to accomplish in terms of which changes are related to missing DegreeGroupedPermutation records versus performance versus unrelated infra updates.
I'll probably take a back seat here unless specific questions come up I can help with.
There was a problem hiding this comment.
why creating a second environment specification instead of using environment.yml?
4 participants
updates to handle missing DegreeGroupedPermutation records and tune performance