events to lightcurve

[JawadAhmadCS]

Issue #28

Use Case

here how this feature use after addition
🔗 Highlighted Use Case Notebook: Notebook Link

---

🔗 Julia Workflow Notebook: Notebook Link
🔗 Python Notebook (Stingray lightcurve usecase for understandng): Notebook Link

@fjebaker @kashish2210 Please review

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.77%. Comparing base (cddac34) to head (215217b).
Report is 21 commits behind head on main.

Files with missing lines	Patch %	Lines
src/lightcurve.jl	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #31      +/-   ##
==========================================
+ Coverage   95.24%   95.77%   +0.53%     
==========================================
  Files           3        5       +2     
  Lines         505      545      +40     
==========================================
+ Hits          481      522      +41     
+ Misses         24       23       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[fjebaker]

Hi @JawadAhmadCS ! This is a great start! I've left a few comments about cleaning up parts of your implementation to make it more Julia-idiomatic, and about handling your tests.

[fjebaker]

Just a handful of little things, then this is ready to merge!

[matteobachetti]

Thanks @JawadAhmadCS!
Some comments on the current implementation

[JawadAhmadCS]

@matteobachetti @fjebaker please review i have resolve all..sorry for the delay in resolving this. I had a fever and some assignments to complete, so it took me a little longer.

[JawadAhmadCS]

Just a handful of little things, then this is ready to merge!

this is now a fully optimized solution.Please review @fjebaker @matteobachetti

[fjebaker]

Just one small suggestion, but it's not vital. Rest looks good! I haven't double checked your expected test cases, but they seem sensible enough 👍

[fjebaker]

I @JawadAhmadCS I can see that these changes are well intentioned, but they are in my opinion too defensive. Many of the checks you add are already handled by fit(Histogram, ...), and allowing edge-cases like this can sometimes make someone's workflow easier. Since none of these return erroneous / wrong results, it's fine to just let them run through.

[JawadAhmadCS]

please review @fjebaker 🤯

[JawadAhmadCS]

Hi @fjebaker, I really appreciate your feedback! right now i am doing 3 internships along with university work, so it's a bit burden. but still trying to contribute whenever I can. by April 15, all my internships will be over, and I'll be totally free to contribute more actively. i am sure you understand

[fjebaker]

Hi @JawadAhmadCS no problem! I am swamped in my own work so have been using the odd bits of spare time to review your PRs. There's no urgency with this, so prioritize your other commitments and we can iterate slowly :)

[JawadAhmadCS]

I think initial approved approach was correct, so I have reverted to that version. Please review and merge if everything looks good—it's fully okay.

[JawadAhmadCS]

@fjebaker please review

[fjebaker]

Hi @JawadAhmadCS! Apologies it has taken a while to get round to finishing this review. This all looks good to me, I will ping @matteobachetti, as he needs to approve the changes before the merge conditions are met.

[JawadAhmadCS]

@fjebaker thanks a lot for the review and approval

[JawadAhmadCS]

@matteobachetti whenever you get a chance, please take a look would love to get this merged