search

waqasbhatti / astrobase

Python modules for light curve work and variable star astronomy
MIT License
55 stars 12 forks source link

Use correct period with periodepsilon in zgls.py #79

Closed joshuawallace closed 5 years ago

joshuawallace commented 5 years ago

Noticed this, maybe I'm wrong but it made more sense for me that the bestperiodsdiff values be compared with the current period, not the previous period. I haven't checked whether a similar algorithm is used in the other period search algorithms but if so and this change is valid, those should be changed too.

waqasbhatti commented 5 years ago

Ah, I think you're right. Thanks for noticing this! I'll apply this to the other methods as well.

waqasbhatti commented 5 years ago

Wait, now I'm not sure anymore. Fractional difference between a current and previous value is (current - previous)/previous I think. periodepsilon is the minimum allowed fractional difference, so I think this is correct:

perioddiff > (periodepsilon*prevperiod)
waqasbhatti commented 5 years ago

I'll revert this merge for now. Is the current method not working as you expected?

joshuawallace commented 5 years ago

Checking the difference between the previous and current period is accomplished in line 590. It seems that line 591 s intended to check the current period against all of the previously found best peaks (at least bestperiodsdiff compares the difference between the current period and the best periods), so I thought the relevant fractional value to compare would be fractional value of the current period rather than the previous period.

I didn't notice it from a behavior thing. I was just looking at it closely to adapt it for some plots I was making last night.

waqasbhatti commented 5 years ago

Yep, I think I see it now. Your change was correct then. I'll apply this elsewhere in periodbase then. Thanks!