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

Add weights support to qffa search #169

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

YaoYLastro
Copy link

  • Added a weights parameter to the folding algorithms for handling light curve intensities.
  • Modified functions search_with_qffa and search_with_qffa_step to support weights input.

Copy link
Member

@matteobachetti matteobachetti left a comment

Choose a reason for hiding this comment

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

@YaoYLastro thanks for your contribution!
I'd like you to write a test showing that the algorithm works as expected, e.g. creating a time series with a pulsation that gets recovered correctly by the algorithm.
I fear that, in the current form, the algorithm with have a lot of side effects if you intend the weights as photon counts, probably not as much if you consider weights à la Fermi (e.g. probability that a given photon is from the source).

@@ -806,11 +807,16 @@ def search_with_qffa_step(
else:
phases = _fast_phase(times, mean_f)

# If weights are not provided, assume uniform weights of 1
if weights is None:
Copy link
Member

Choose a reason for hiding this comment

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

If I'm not mistaken, the algorithm should work even if you keep the weight keyword to None. If you force it to be np.ones, you add an additional multiplication in all steps that most of the time will be unneeded, making the algorithm a lot less efficient in the typical use case.

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