Skip to content

Hotfix geom graph combine take2#302

Merged
aarograh merged 13 commits intomasterfrom
hotfix_geom_graph_combine_take2
Feb 1, 2021
Merged

Hotfix geom graph combine take2#302
aarograh merged 13 commits intomasterfrom
hotfix_geom_graph_combine_take2

Conversation

@aarograh
Copy link

This rewrites the GraphType combine method to account for several robustness issues.

@aarograh aarograh requested a review from HendersonSC January 28, 2021 20:57
@aarograh aarograh self-assigned this Jan 28, 2021
Copy link
Contributor

@HendersonSC HendersonSC left a comment

Choose a reason for hiding this comment

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

Overall good start...You already know about the debug statements so I will give you a chance to clear those up before commenting on them.

It looks like you have added some comments on how it all works, but the is large enough its hard to expand it and make sure it all flows. I will try to look at that in a little more detail on a second look.

@HendersonSC
Copy link
Contributor

As far as comments and process go, it is clearly better. I don't know where you want to draw the line, someone just getting into graphs might not fully keep up, but I think generally between the naming and comments its in a decent place.

@aarograh
Copy link
Author

Alright, I think this is all set if you want to give it a final look over @HendersonSC . The only test that failed is testGenReqTables, which has nothing to do with my changes. The docker container downloads a bunch of python packages each time it runs and we think that one of the packages got changed in a way that broke that test.

@aarograh
Copy link
Author

aarograh commented Feb 1, 2021

the failed test is testGenReqTables due to python package changes and has nothing to do with these changes. Going ahead with the merge.

@aarograh aarograh merged commit 8d9b1b0 into master Feb 1, 2021
@aarograh aarograh deleted the hotfix_geom_graph_combine_take2 branch February 1, 2021 15:06
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