eclipse / chemclipse

ChemClipse Project
Eclipse Public License 1.0
33 stars 18 forks source link

Deprecate ion limit and abundance exceptions #1713

Closed Mailaender closed 3 months ago

Mailaender commented 3 months ago

My main ideas here are

It is therefore imperative to keep their footprint low. The AbundanceLimitExceededException and IonLimitExceededException therefore seem like a bad idea, because

What happened were redundant checks if float abundance is inside FLOAT.MIN_VALUE and FLOAT.MAX_VALUE sometimes even with an additional if-clause inside the try catch for the exception.

Therefore, I replaced it with a boolean return in setAbundance and setIon so exceptions aren't used for control flow because that is an anti-pattern, but we retain the logic in the filters where the exceptions weren't just logged and ignored.

I also found this useful during converter development to check for invalid and therefore highly likely wrong parsings, but this can easily be recreated in the parser and should also possibly be removed afterward.

eselmeister commented 3 months ago

That's a good idea. The ion and abundance limit system can be dramatically simplified. It should be delegated to the export converter to constrain the range of possible m/z and abundance values.

It's a massive change, which needs to be applied with backward compatibility. Hence, it's a boss job. Let me do it as I have invented the system in the beginning of the software and know exactly how and where to change it.