Additional clean up after initial alpha push - no new functionality#435
Conversation
I deleted the DRAFT on the title, and I deleted all of the random stuff at the end of the document and moved that content into a placeholder for the eventual release notes.
those changes - logic cleanup to make the code more readable and maintainable. No changes in functionality.
peterrrock2
left a comment
peterrrock2
left a comment
There was a problem hiding this comment.
This is looking good so far! Couple of small nitpicks in here and then responses to your comments
| # frm: TODO: Documentation: more detail on contents of JSON file needed above in docstrings | ||
|
|
| # frm: TODO: Refactoring: Encapsulate ALL NX dependencies in this file. | ||
|
|
||
| # frm: TODO: Refactoring: Move routines so that the ones that do NOT mirror NetworkX | ||
| # come at the end, after a comment to that effect. | ||
|
|
||
| # Note: The two laplacian functions: laplacian_matrix() and | ||
| # normalized_laplacian_matrix() are part of the Graph class primarily to | ||
| # encapsulate all NetworkX dependencies in one place - this module. |
There was a problem hiding this comment.
This all sounds good to me
| restarts = 0 | ||
| attempts = 0 | ||
|
|
||
| # frm: TODO: Code: When would it make sense for max_attempts to be None? Infinite loop potential... |
There was a problem hiding this comment.
Yeah, the None value is terrible. We should force it to be an int and put a defensive check before this.
| # frm: TODO: Refactoring: Remove old_bipartition_tree_random() after PR code review | ||
| # | ||
| # A while ago I added the routine, bipartition_tree_random_with_num_cuts() because | ||
| # the routine, reversible_recom() in tree_proposals.py had used an internal routine | ||
| # called _get_possible_edge_cuts_and_populated_graph() because it wanted to know how many possible | ||
| # cut solutions existed. I did what was expedient and just added a function inside | ||
| # tree.py that did what the code in tree_proposals.py did in order to hide the | ||
| # internal routine. I later realized that it was EXACTLY the same as bipartition_tree_random() | ||
| # except that it passed back one more value, so I just replaced the definitioin of | ||
| # bipartition_tree_random() with a call to bipartition_tree_random_with_num_cuts() and | ||
| # then discarded the extra num_cuts returned value. | ||
| # | ||
| # I am leaving this in the codebase for now, just to make it easier for Peter to see | ||
| # what is going on when he does a review. Note that after this change all tests passed | ||
| # (January 9, 20206) |
| # frm: TODO: Refactoring: Would be nice to rename "method" to "bipartition_tree_method" | ||
| # | ||
| # This is clearly not a high priority, but it would be nice to have a parameter name that | ||
| # better stated what it meant. "method" indicates that it is a function, but what does | ||
| # the function do? When reading the code where "method" is called, it would have been | ||
| # nice to see bipartition_tree_method(...) instead. | ||
| # | ||
| # However, for legacy code reasons it is probably not worth changing. *sigh* | ||
| # | ||
| # Peter - please confirm that this is not something that is worth changing, and | ||
| # then I will be able to put it behind me ;-). |
There was a problem hiding this comment.
Nah, go ahead and change this. Changing the method in the epsilon_tree_bipartition is rare, and I am willing to deal with any emails coming from the update.
| # frm TODO: Refactoring: recursicve_seed_part() is never called - not in this file and not in any other | ||
| # GerryChain file. Is it intended to be used by end-users? | ||
| # | ||
| # It calculates an initial assignment dictionary - for use in creating a Partition object. | ||
| # | ||
| # Are there other uses as well? The comment for recursive_tree_part() implied that there | ||
| # might be other uses for creating an initial assigment-like dict... |
There was a problem hiding this comment.
I am unsure of what was in the mind of the creator of this function, but I have only ever seen it used for making initial partitions of a graph.
There was a problem hiding this comment.
Moving recursive_seed_part, recursive_seed_part, and epsilon_tree_bipartition makes sense to me. My only organizational note is that I would like the recursive functions placed in something like "partition/initial_paritition_generators.py" rather than in "parition/partition.py" since they have been used for things outside of making a Partition object before, and we have some more algorithms that we might add to the mix later this year (also, I would like to try and move this repository to the "one thought per file" paradigm whenever we get the chance during this refactor). The epsilon_tree_bipartition is fine to go in "proposals/tree_proposals.py", however.
There was a problem hiding this comment.
Note to self: we will need to make sure to add this to the top of the release notes.
Co-authored-by: Peter <27579114+peterrrock2@users.noreply.github.com>
Co-authored-by: Peter <27579114+peterrrock2@users.noreply.github.com>
Co-authored-by: Peter <27579114+peterrrock2@users.noreply.github.com>
Co-authored-by: Peter <27579114+peterrrock2@users.noreply.github.com>
Co-authored-by: Peter <27579114+peterrrock2@users.noreply.github.com>
|
Peter, I closed the PR, thinking that doing so would trigger an auto-merge, but when that did not seem to happen, I re-opened it. Note that I committed all of your proposed changes. I assume that it is up to you to do the merge. When you have done so, I will do a pull - just to be sure everything is synched, and then I will go back to making changes. -Fred |
|
No problem. I'll merge this in now. |
2 participants
Lots of lines that have changed - hopefully not too painful.
There is a block comment at the top of tree.py that you should look at. Tree.py is the file that got serious refactoring done. Most of the rest of the changes were of the clean-up variety.
Looking forward to your feedback..
-Fred