Complete rewriting of PlanarDiagram::GetSmallGirthMorseCode

[th-rtyf-re]

I've completely rewritten the function PlanarDiagram::GetSmallGirthMorseCode in order to improve readability and also address some issues I discovered while reading through the code. In its current state, I'm not sure if this fork should be merged, for reasons I will state further below, but I hope that this can be discussed.

The new function(s) are closely related to the previous function. I made the following changes:

1. I separated the function into a main function and two auxiliary functions for a better presentation of the different steps. The two auxiliary functions are get_max_connections, which finds maximally connected crossings to attach to a partial Morse list, and extend_Morse_list, which actually adds the next crossing.
2. I gave variables more descriptive names and added comments describing each step of the function.
3. The original function would find maximally connected crossings and attempt to attach them even if the new crossing's adjacent edges were not in the right order in the list of edges at the bottom of the upper knot diagram: that is, the function would attach crossings that could not be attached with edges ordered counterclockwise, as specified by the planar diagram. This led to attempts to build a Morse list that would always fail, but which were not abandoned immediately. I added some code to check that crossing candidates could be attached with counterclockwise edges.
4. Failed attempts were still taken into account when updating the smallest Morse list and smallest girth. If an attempt failed early on, it might make the smallest girth too small, thereby invalidating all future Morse list candidates. This was fixed with a goto statement.
5. I removed a bit of randomness: the choice of first crossing and first orientation is now done in lexicographical order. Since the number of choices for this first part is 4 * n_crossings, which will generally stay reasonably small, I don't think this will greatly affect the ability to reach all possible Morse lists by calling the function several times. Moreover, this guarantees that the function will try starting with every single crossing early on, which I believe reduces the chance of finding the same Morse list several times early on.

The reasons that I would like to discuss these changes are the following:

1. I am not actually too familiar with how to do version control properly, so maybe this rewriting is the wrong way to go.
2. I have completely rewritten the function. Most of the function is still structurally the same as before, just with different variable names, but the check for counterclockwise edges is new, and in the process of rewriting I may have introduced new errors. I have tested the new function a little bit with no errors, but it's hard to tell if it is actually correct. What would be good tests for this function?
3. When rewriting the function, I used the coding style that I am most comfortable with, e.g. lower_case_with_underscores for variables. This doesn't match the rest of the code. Should this be changed?
4. I do not have the means to compile the code on different operating systems, and in particular I don't know if other OSs will complain about integer conversions, so that would need to be checked before merging.
5. I've used goto twice, in situations where I didn't see a more elegant solution. They should be relatively acceptable uses, since in both cases I used it to break out of nested loops. Once again, I don't know if there are any coding guidelines, but this is something to keep in mind for future development.

Sorry for the long request... another much faster fix would be changing the continue of 79ce6ff to a goto AttemptEnd and then adding AttemptEnd: ; on line 205 (after the code saving the improved Morse list). If that's better, then I can make another pull request with that change. It doesn't address all the issues, but it should correspond to change 4 above.

[th-rtyf-re]

Regarding the failed checks:

Okay so I am discovering things as I go along, but I've also run python3 -m knot_floer_homology.test and it fails on what seems to be the first knot, the trefoil: expected [(-1, -2), (0, -1), (1, 0)], got [(0, -1)]. However, if I manually test this case, I get the correct homology. What am I doing wrong?

[culler]

Thanks for working on this Isaac. I have a superficial comment on your comments, not based on looking at the code. I think we would come to regret intermingling two different styles for variable names. I think it would be best to use the same style for your new variables that is used in the rest of the package. Also that would probably be an easy correction to make at this point, but possibly painful after a few years.

[th-rtyf-re]

Thank you Marc for your quick reply! I've changed the style to something more coherent (although it's unclear if I should go for UpperCamelCase or lowerCamelCase).

[culler]

I see what you mean. It seems like there is no convention for variables that do not have descriptive names and that the upper case form is much more prevalent for descriptive names. So I think your choice is optimal given the context.