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

Deprecate ion limit and abundance exceptions #1713

Merged
merged 2 commits into from
Apr 2, 2024

Conversation

Mailaender
Copy link
Contributor

@Mailaender Mailaender commented Mar 20, 2024

My main ideas here are

  • logging and exception handling is expensive
  • ions are the smallest and most abundant type we have

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

  • we don't know what the actual limits are and the file formats can never know
  • they can actually be changed by instrument maintenance (like cleaning the ion source)

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.

@Mailaender Mailaender force-pushed the ion-exceptions branch 3 times, most recently from cf0d578 to 17d961e Compare March 20, 2024 18:45
@eselmeister
Copy link
Contributor

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.

@eselmeister eselmeister merged commit 880e65b into eclipse:develop Apr 2, 2024
3 checks passed
@Mailaender Mailaender deleted the ion-exceptions branch April 2, 2024 11:01
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.

None yet

2 participants