-
Notifications
You must be signed in to change notification settings - Fork 33
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
Quantifiable interfaces for results reading in FlashLFQ #793
Conversation
…gional IQuantifiable in FlashLFQ
…g full file paths
…gger file name additions and wrote tests
…bilty v peptideprophetprobability)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #793 +/- ##
==========================================
+ Coverage 75.48% 75.52% +0.04%
==========================================
Files 201 202 +1
Lines 30857 30945 +88
Branches 3112 3129 +17
==========================================
+ Hits 23292 23371 +79
- Misses 7039 7040 +1
- Partials 526 534 +8
|
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.
An excellent display of object oriented programming. Best first time PR that I have seen to date.
I have a few suggestions, but nothing major. Comments were main stylistic and simple things to improve readability and usability.
Excess white space can be removed at the end of many files.
string proteinAccessions; | ||
string geneName; | ||
string organism; | ||
|
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.
Add comments here!
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.
Specifically, you should mention that what happens here is also done in UsefulProteomicsDatabases/ProteinDatabaseLoader. i.e., you're parsing a fasta header. What you have is fine for now, but ideally, a future refactor would create a method for parsing fasta headers that is shared by Readers and UsefulProteomicsDatabases
There should be more comments at the level of whole classes/interfaces. Like, the descriptions you have in the summary comment for this PR are great. If you just copy and paste those into the top of their respective interfaces/classes, that would be perfect |
using System.Text; | ||
using System.Threading.Tasks; | ||
|
||
namespace FlashLFQ.ResultsReading |
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.
Change the namespace to just FlashLFQ
mzLib/Readers/ExternalResults/BaseClasses/IQuantifiableRecord.cs
Outdated
Show resolved
Hide resolved
/// <summary> | ||
/// MsFragger v22.0 output renames the header "PeptideProphet Probability" as just "Probability". | ||
/// Headers are mutually exclusive, will not both occur in the same file. | ||
/// As such, both instances need to be accounted for seperately as optional fields. |
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.
You should delete this line (100)
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.
Great work, its almost there
Overall: created interfaces that enable external result files to be usable by FlashLFQ through the IdentificationAdapter
Interfaces
Implementation
Classes