-
Notifications
You must be signed in to change notification settings - Fork 53
Add option for a custom alignment file in caprieval #1418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
AnnaKravchenko
wants to merge
16
commits into
main
Choose a base branch
from
cust-alig-capri
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
d31a0a3
new parameter alig_fname added to defaults
AnnaKravchenko e4b59e2
add align_custom to libalign
AnnaKravchenko 03067d5
upd run() of caprieval
AnnaKravchenko 24f8295
sneaky typos!
AnnaKravchenko 869dddd
fix current tests
AnnaKravchenko 018e961
better parameter description
AnnaKravchenko a1dfbd1
change explevel to expert
AnnaKravchenko 4a95870
starting on tests
AnnaKravchenko 1eb8339
add example izone file
AnnaKravchenko 15548fd
Merge branch 'main' into cust-alig-capri
AnnaKravchenko b5b1301
Merge branch 'main' into cust-alig-capri
AnnaKravchenko f12a685
Merge branch 'main' into cust-alig-capri
AnnaKravchenko 7f9f138
Merge branch 'main' into cust-alig-capri
AnnaKravchenko 65922e9
Merge branch 'main' into cust-alig-capri
AnnaKravchenko b15271c
rename alig_fname to align_fname
AnnaKravchenko f80c05c
rename alig_fname to align_fname v2
AnnaKravchenko File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| # protein | ||
| ZONE A-1:A-1 | ||
| ZONE A0:A0 | ||
| ZONE A1:A1 | ||
| ZONE A2:A2 | ||
| ZONE A3:A3 | ||
| ZONE A4:A4 | ||
| ZONE A5:A5 | ||
| ZONE A6:A6 | ||
| ZONE A7:A7 | ||
| ZONE A8:A8 | ||
| ZONE A9:A9 | ||
| ZONE A10:A10 | ||
| ZONE A11:A11 | ||
| ZONE A12:A12 | ||
| ZONE A13:A13 | ||
| ZONE A14:A14 | ||
| ZONE A15:A15 | ||
| ZONE A16:A16 | ||
| ZONE A17:A17 | ||
| ZONE A18:A18 | ||
| ZONE A19:A19 | ||
| ZONE A20:A20 | ||
| ZONE A21:A21 | ||
| ZONE A22:A22 | ||
| ZONE A23:A23 | ||
| ZONE A24:A24 | ||
| ZONE A25:A25 | ||
| ZONE A26:A26 | ||
| ZONE A27:A27 | ||
| ZONE A28:A28 | ||
| ZONE A29:A29 | ||
| ZONE A30:A30 | ||
| ZONE A31:A31 | ||
| ZONE A32:A32 | ||
| ZONE A33:A33 | ||
| ZONE A34:A34 | ||
| ZONE A35:A35 | ||
| ZONE A36:A36 | ||
| ZONE A37:A37 | ||
| ZONE A38:A38 | ||
| ZONE A39:A39 | ||
| ZONE A40:A40 | ||
| ZONE A41:A41 | ||
| ZONE A42:A42 | ||
| ZONE A43:A43 | ||
| ZONE A44:A44 | ||
| ZONE A45:A45 | ||
| ZONE A46:A46 | ||
| ZONE A47:A47 | ||
| ZONE A48:A48 | ||
| ZONE A49:A49 | ||
| ZONE A50:A50 | ||
| ZONE A51:A51 | ||
| ZONE A52:A52 | ||
| ZONE A53:A53 | ||
| ZONE A54:A54 | ||
| ZONE A55:A55 | ||
| ZONE A56:A56 | ||
| ZONE A57:A57 | ||
| ZONE A58:A58 | ||
| ZONE A59:A59 | ||
| ZONE A60:A60 | ||
| ZONE A61:A61 | ||
| ZONE A62:A62 | ||
| # DNA | ||
| ZONE B1:B1 | ||
| ZONE B2:B2 | ||
| ZONE B3:B3 | ||
| ZONE B4:B4 | ||
| ZONE B5:B5 | ||
| ZONE B6:B6 | ||
| ZONE B7:B7 | ||
| ZONE B8:B8 | ||
| ZONE B9:B9 | ||
| ZONE B10:B10 | ||
| ZONE B11:B11 | ||
| ZONE B28:B28 | ||
| ZONE B29:B29 | ||
| ZONE B30:B30 | ||
| ZONE B31:B31 | ||
| ZONE B32:B32 | ||
| ZONE B33:B33 | ||
| ZONE B34:B34 | ||
| ZONE B35:B35 | ||
| ZONE B36:B36 | ||
| ZONE B37:B37 | ||
| ZONE B38:B38 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This here is a bit odd, the function is deciding between structural/sequence alignment - what is the type of this new alignment you are adding?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
align_fnameis a path to a custom alignment file - for those extremely rare cases when haddock cannot handle alignment, e.g. short DNA and extended DNA.Here I am avoiding adding new allowed value in
alignment_methodparameter (would be smth likealignment_method = custom). Instead, I am adding a new parameteralign_fname. And so the idea is to chek if user provided a custom alignment file - and also that this file exists - before checkig for the value inalignment_method.Does it make sence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But not that I am checking - no need for the FileNotFoundError here,
libutilwill take care of it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, this is actually a good case for
kwargs;Then this
get_aligncan be called as:(I typed the code out of my head maybe the syntax is wrong)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation in my point of view is cleaner and simpler, *in your implementation you are effectively triggering a new type of method by using a parameter which is not "method", here you are using
align_fnameinstead - you see the complexity?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and no. I do see how the code is cleaner, but I also see a new dependency between
method == “custom”andalign_fname=fname.In practice, most if not all users will specify
align_fname, whilemethodwill remain at its default, so “sequence” - this is smth we’ve seen resently with the per-interface-scoring PR. Sure, it’s possible to handle this mismatch, print error message etc., but wouldn’t it be more straightforward to ignoremethodvar wheneveralign_fnameis defined? Given I add comment in the code to explain this behavior?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The frequency (or the practice) of a code pathway is not really relevant as the code exists regardless of how many times it is used.
This
get_alignis a factory function, and the proposed implementation will break this design. So it's not a matter of whether this is documented or not, it's more about design patterns. My suggestion is to stick to the patterns, as they exist for a reason.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then how do you interupt the run if
align_fname=fnameandmethod == “ sequence”?Raising error in libparallel.py just logs warnings, but the run keeps going.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this is also by design, parameter validation is done upstream. For this specific case this can be a method in the
CAPRIclass: