Skip to content

fix: Extending TS can now create lower cost alignment without panic#90

Merged
sebschmi merged 1 commit intoalgbio:mainfrom
jeeeesper:dont-panic-on-extend
Nov 3, 2025
Merged

fix: Extending TS can now create lower cost alignment without panic#90
sebschmi merged 1 commit intoalgbio:mainfrom
jeeeesper:dont-panic-on-extend

Conversation

@jeeeesper
Copy link
Copy Markdown
Collaborator

@jeeeesper jeeeesper commented Jun 11, 2025

Apart from some reformatting, the only change is that the assertions on the new cost being equal are removed.

Closes #89

@jeeeesper jeeeesper marked this pull request as draft June 11, 2025 08:25
@sebschmi
Copy link
Copy Markdown
Collaborator

Nice, thanks! I propose that we replace the former assertions assert_eq!(new_cost, initial_cost); with initial_cost = new_cost, to avoid increasing the cost again, as described in the issue.

@jeeeesper jeeeesper force-pushed the dont-panic-on-extend branch 2 times, most recently from d65fc15 to 5ccfe6a Compare June 13, 2025 10:13
@jeeeesper jeeeesper requested a review from sebschmi June 13, 2025 13:02
@jeeeesper
Copy link
Copy Markdown
Collaborator Author

Hey @sebschmi , this should be fine and all tests pass, but maybe you wanna take a look before we merge. Also, I've done a bit of refactoring to avoid panics in general, to allow for more graceful failure.

@jeeeesper jeeeesper marked this pull request as ready for review June 13, 2025 13:03
Copy link
Copy Markdown
Collaborator

@sebschmi sebschmi left a comment

Choose a reason for hiding this comment

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

Thanks, looks very nice. Two of the comments you made indicate a bug to me, as I commented at the specific lines. This kind of bug is something we may actually run into, in case an alignment starts with a TS, or in case the TS inner is at the boundary of the sequence.

@jeeeesper jeeeesper force-pushed the dont-panic-on-extend branch 2 times, most recently from 0c9a1db to 8d40aa6 Compare June 24, 2025 13:49
@jeeeesper jeeeesper requested a review from sebschmi June 25, 2025 08:53
Copy link
Copy Markdown
Collaborator

@sebschmi sebschmi left a comment

Choose a reason for hiding this comment

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

Thanks! I went over things again, and noticed that we would now panic in some cases in debug mode, but allow them in release mode. We (I) never run this code in debug mode though, because it is too slow. So best to still either panic when the assert indicates a programming error within the function, or return a Result::Err in case of violated preconditions of the method. But in case of violated preconditions, we can also just document that false is returned. But then again we may not want to panic in debug mode then...

I think cleanly using either asserts or return Result::Err is the best way to deal with these.

I marked some of the spots of this happening in the first method, but since the others are similar I didn't mark them.

@jeeeesper
Copy link
Copy Markdown
Collaborator Author

I've implemented it now so that false is returned on error since the docs imply that anyway, and an error is logged in that case.

@jeeeesper
Copy link
Copy Markdown
Collaborator Author

And now it's even rebased 🫡

@jeeeesper jeeeesper mentioned this pull request Sep 18, 2025
@sebschmi sebschmi merged commit 7fc3f1f into algbio:main Nov 3, 2025
18 checks passed
@jeeeesper jeeeesper deleted the dont-panic-on-extend branch November 3, 2025 09:18
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.

Extending a template switch finds a better alignment

2 participants