search

3-manifolds / SnapPy

SnapPy is a package for studying the topology and geometry of 3-manifolds, with a focus on hyperbolic structures. It is based on the SnapPea kernel written by Jeff Weeks.
https://snappy.computop.org/
87 stars 41 forks source link

fix one wrong code line #64

Closed fchapoton closed 2 years ago

fchapoton commented 2 years ago

sorry, I have not tested, but it was such an obvious wrong line..

culler commented 2 years ago

I agree that it is pointless to include that line in a program since it has no effect. However, I seriously doubt that the correct fix for a command that has no effect is likely to be changing it into a command which does have an effect. If the program worked correctly when that command was in place then the correct fix would be to remove the command altogether. If the IP.more variable actually did always need to be set to True at that place then there should be some reason, meaning that there should be some wrong behavior which would become correct when that variable gets set to True. On the other hand, if that variable sometimes needed to have the value False, then assigning it the value True on that line will break something. I don't see how a change like that can be made without understanding the consequences.

Removing the command is guaranteed not to make the program worse, but it also removes a hint that there may be a bug hiding in there. I would prefer that it be commented out and a note added to the comment to remind us to look at it.

On Sun, Jun 26, 2022 at 1:11 PM Frédéric Chapoton @.***> wrote:

sorry, I have not tested, but it was such an obvious wrong line..

— Reply to this email directly, view it on GitHub https://github.com/3-manifolds/SnapPy/pull/64#issuecomment-1166605281, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJ6CP3WYAR43NFJL2HISN3VRCMNPANCNFSM5Z3NZRIQ . You are receiving this because you commented.Message ID: @.***>

NathanDunfield commented 2 years ago

I agree with Marc that actual testing is needed here. I did so, and confirmed this patch changes the behavior. For example, if you hit return three times in the unpatched version you get:

In[1]: 

In[1]: 

In[1]: 

In[1]:

where as with the patch you get:

In[1]: 

In[1]: 
  ...:

This do-nothing line is from a recent commit (cc63a2c6b), so not included in any recent release.

culler commented 2 years ago

Thank you, @NathanDunfield ! I think that means that the line in question should be removed.