-
Notifications
You must be signed in to change notification settings - Fork 8
05/06/fea amino acid properties #5
base: master
Are you sure you want to change the base?
05/06/fea amino acid properties #5
Conversation
chrismacklin
left a comment
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.
Nice improvement, will merge after tweaks.
src/AmyrisBio/biolib.fs
Outdated
| (End, { trigram="End" ; letter = '*' ; hydrophobicity = None }) | ||
| ] |> Map.ofList | ||
|
|
||
| let aaLetterToSymbolic (aa:char) = |
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.
Please rename as aaLetterToSymbol for consistency,
src/AmyrisBio/biolib.fs
Outdated
| | 'Y' -> Tyr | ||
| | 'W' -> Trp | ||
| | '*' -> End | ||
| | _ -> failwithf "ERROR: unknown amino acid letter '%c' in aaLetterToSymbolic" aa |
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.
Could we use the Result type here instead of raising an exception?
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.
If it must be an exception, let's either use a more specific exception class or declare a custom exception type.
src/AmyrisBio/biolib.fs
Outdated
|
|
||
|
|
||
| /// Translates amino acid represented with one character to a trigram, eg 'W' -> "Trp" , '*' -> "End" | ||
| let aaTrigramToSymbolic (aa:string) = |
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.
aaTrigramToSymbol
src/AmyrisBio/biolib.fs
Outdated
| | "Tyr" -> Tyr | ||
| | "Trp" -> Trp | ||
| | "End" -> End | ||
| | _ -> failwithf "bad aa '%s' in aaLetterToSymbolic" aa |
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.
Result type here as well would be nice.
…mments MIN: addressed review comments
@chrismacklin , i have addressed all your comments and updated this PR. I think this PR would be easier to be merged before the migration to .net core. |
This is a small extension to the amino acid representation / handling, a severely neglected corner of our basic representation, that puts the letter e.g. 'K' , symbolic Lys and trigram "Lys" forms on same footing and allows interconversion. Also pulls in some common properties and references. I keep on rewriting standalone versions of this all the time, so contributing back something more permanent