Skip to content
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

Proforma v2 from HUPO-PSI #71

Merged
merged 24 commits into from
Sep 28, 2020
Merged

Proforma v2 from HUPO-PSI #71

merged 24 commits into from
Sep 28, 2020

Conversation

rfellers
Copy link
Member

NOT READY TO MERGE

This large PR does the following things:

  • Removes U of W specific types (like we did for NU code)
  • Adds isotope support to chemical formulas
  • Updates to .NET standard 2.1 and turns on nullable ref types
  • Implements draft HUPO-PSI ProForma v2 spec

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.

…lag to ProFormaParser (not being used just yet), adds notes to the unit tests for ProForma v2
@rfellers rfellers requested review from acesnik, leahvschaffer and a team September 16, 2020 19:49
@rfellers rfellers mentioned this pull request Sep 16, 2020
@acesnik
Copy link
Contributor

acesnik commented Sep 17, 2020

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

acesnik commented Sep 23, 2020

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
Copy link
Member Author

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
Copy link
Contributor

acesnik commented Sep 28, 2020

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

@acesnik acesnik merged commit 8565fd6 into master Sep 28, 2020
@acesnik acesnik deleted the proforma-hupo-psi branch September 28, 2020 15:37
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.

2 participants