Concrete Chemical Formula

topdownproteomics / sdk

Software solution for common top-down proteomics tasks

http://www.topdownproteomics.org/

MIT License

9 stars 4 forks source link

Concrete Chemical Formula #104

Closed rfellers closed 1 year ago

rfellers commented 1 year ago

Removes IChemicalFormula in favor of a concrete ChemicalFormula class only. Removing flexibility point allows for increased performance of this low-level class because we don't need to make generic Equals() and GetHashCode() implementations. Obvious downside is that users won't be able to make their own implementation of IChemicalFormula and pull it in. Please let me know your thoughts!
Implements a common vs. uncommon element approach in chemical formula to make the common path very fast while keeping memory overhead low. The six elements used in amino acids are stored in dedicated arrays to make all operations faster. If other elements are used, they are stored in an uncommon list. Everything is created as needed to keep memory pressure low.

rfellers commented 1 year ago

thanks @acesnik! In the most recent version of our internal libraries, we took a dependency on TD SDK. No better way to find problems than to dogfood it, right?! This is how we ran into this performance problem. This PR approach is moving in the direction of mzLib (concrete class, no interface), but it is immutable where mzLib's can be changed. I personally think this is a safer way to go, but I'm up for the debate! Thanks again.

acesnik commented 1 year ago

I think you're right that it's safer. I'm not sure if we actually have an instance where we're changing the formula; they're usually just instantiated with a different formula.

acesnik commented 1 year ago

Feel free to merge this. We can discuss mzLib integration details on a different forum if needed.

acesnik commented 1 year ago

It looks like you still have NETStandard2.1 cases in there; can these be removed, since that framework is deprecated?

rfellers commented 1 year ago

It looks like you still have NETStandard2.1 cases in there; can these be removed, since that framework is deprecated?

We added multiple targeting on library awhile back to support .NET 4.7.2, so that's where all the conditional #ifdefs are coming from. Please correct me if I'm wrong, but I don't think .NET Standard is deprecated, but more not being updated. If one isn't using features beyond .NET Standard 2.1, I think it is still the best target to use to support .NET Core 3.1 and newer.