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

Spectral averaging with outlier rejection #28

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

Janne98
Copy link
Collaborator

@Janne98 Janne98 commented Oct 25, 2024

Add option to choose consensus spectrum method by setting --consensus_method to medoid or average. If average, spectral averaging with outlier rejection will be performed.

@Janne98 Janne98 marked this pull request as ready for review November 3, 2024 11:59
@Janne98 Janne98 requested a review from bittremieux November 3, 2024 11:59
@bittremieux
Copy link
Member

@Janne98 Can you first make sure that merge conflicts are resolved?

@Janne98
Copy link
Collaborator Author

Janne98 commented Nov 5, 2024

@Janne98 Can you first make sure that merge conflicts are resolved?

Done

Copy link
Member

@bittremieux bittremieux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments.

In addition, the averaging implementation seems a bit complex at certain points, with creating empty lists of NumPy arrays that are grown iteratively. This is not really optimal; maybe there are some possibilities to still simplify that a bit.

falcon/cluster/cluster.py Outdated Show resolved Hide resolved
falcon/cluster/cluster.py Outdated Show resolved Hide resolved
falcon/cluster/cluster.py Outdated Show resolved Hide resolved
falcon/cluster/cluster.py Outdated Show resolved Hide resolved
falcon/cluster/cluster.py Outdated Show resolved Hide resolved
falcon/cluster/cluster.py Outdated Show resolved Hide resolved
np.ndarray
The array of intensities with outliers removed.
"""
while len(intensities) > 2:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: Interesting, I didn't know this was performed iteratively. Don't you progressively remove a bunch of peaks that way?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is how they describe it in the paper

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into it a bit further and sigma clipping is an iterative procedure by default, but typically removes only 1 outlier per iteration. However, the paper uses a "bulk rejection" version of sigma clipping to limit the number of calculations (median and std).

falcon/cluster/cluster.py Outdated Show resolved Hide resolved
falcon/config.py Outdated Show resolved Hide resolved
falcon/cluster/cluster.py Outdated Show resolved Hide resolved
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