search

topdownproteomics / sdk

Software solution for common top-down proteomics tasks
http://www.topdownproteomics.org/
MIT License
9 stars 4 forks source link

Proforma v2 from HUPO-PSI #71

Closed rfellers closed 3 years ago

rfellers commented 3 years ago

NOT READY TO MERGE

This large PR does the following things:

Please take look (either detailed and high level) and let me know if you have questions/concerns. For those that are interested, I'm happy to walk through what I did and discuss.

acesnik commented 3 years ago

I just skimmed the changes, and it's great to see the examples from the ProForma v2 text in the tests already! The tests for stuff like overlapping ranges is very thorough and good to see.

I didn't see the inter-chain XL semantics in there yet, although maybe I missed it. Could you make Github issues for new features from ProForma2 that have yet to be implemented?

The integration of the UW fine isotopic distribution generator looks great!

You could consider updating all projects to netcore3.1, so they're using the same version. I think there was one I saw that was at netcore2.1, for example. Not a big deal, though.

rfellers commented 3 years ago

Thanks for the feedback @acesnik! I tried to make sure all the examples from the document were in unit tests.

Re: inter-chain XL support, you are correct, I skipped that part. Actually, I explicitly unit test to confirm that it fails. As always, feedback is welcome, but I thought having a simpler API where "one ProForma string => one ProFormaTerm" made more sense than supporting inter-chain XLs (as it appears to be a fringe case at this point).

I'm glad you like the U of W incorporation, that is something I've wanted to do for a long time.

I believe that the projects all target the correct versions of .NET. I think you saw the library targeting .NET Standard 2.1, which is what you want in conjunction with .NET Core 3.x. At some point soon, everything will be .NET 5 and we can forget this version soup!

rfellers commented 3 years ago

AppVeyor is complaining about .NET Standard 2.1 and .NET Core 3.1. Is there a way to use a more up to date SDK during build? I'm happy to switch the build over to what we use internally (Azure DevOps) if that would be easier.

acesnik commented 3 years ago

I just fixed the appveyor build. I see now that in my previous comment I misread netstandard2.1 for netcore2.1; that versioning looks fine.

rfellers commented 3 years ago

Thanks for fixing that build, didn't realize I probably could have done that myself.

@acesnik If you are OK with the decision not to support inter chain XL mods, then this is probably ready to merge. I did add an issue for inter-chain crosslinks, #72.

acesnik commented 3 years ago

Sounds great, Ryan. I'll approve and merge it in. Thanks for opening that issue, too.