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

Extend join operation for events to arbitrary array attributes #742

Merged
merged 12 commits into from Aug 18, 2023

Conversation

matteobachetti
Copy link
Member

No description provided.

@matteobachetti matteobachetti marked this pull request as draft August 4, 2023 08:38
@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Merging #742 (a86d988) into main (bd7dadf) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #742      +/-   ##
==========================================
+ Coverage   97.12%   97.13%   +0.01%     
==========================================
  Files          42       42              
  Lines        7884     7919      +35     
==========================================
+ Hits         7657     7692      +35     
  Misses        227      227              
Files Changed Coverage Δ
stingray/utils.py 98.50% <ø> (ø)
stingray/events.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@matteobachetti matteobachetti marked this pull request as ready for review August 4, 2023 10:56
@matteobachetti matteobachetti marked this pull request as draft August 4, 2023 12:18
@matteobachetti matteobachetti marked this pull request as ready for review August 4, 2023 12:48
Copy link
Collaborator

@mgullik mgullik left a comment

Choose a reason for hiding this comment

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

Hi @matteobachetti,
thank you very much for your contribution!
I listed a few minor comments for your pull request, after you address those, we should be able to approve it.

Thanks!

stingray/events.py Outdated Show resolved Hide resolved
stingray/events.py Outdated Show resolved Hide resolved
stingray/tests/test_events.py Outdated Show resolved Hide resolved
stingray/tests/test_events.py Show resolved Hide resolved
stingray/tests/test_events.py Show resolved Hide resolved
stingray/tests/test_events.py Show resolved Hide resolved
stingray/tests/test_events.py Show resolved Hide resolved
stingray/tests/test_events.py Outdated Show resolved Hide resolved
stingray/tests/test_events.py Outdated Show resolved Hide resolved
@matteobachetti
Copy link
Member Author

@mgullik thanks for the review. I think I addressed all your comments but one, where I'm open to changing the behavior but would prefer leaving it as is.

Copy link
Collaborator

@mgullik mgullik left a comment

Choose a reason for hiding this comment

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

Hi @matteobachetti,
thanks for making the requested changes.

I think we are ready to approve it. I just want to add a bit more of explanation of the Results doc of this routine. Something like this

I think it could
Returns
-------
ev_new : :class:EventList object
The resulting :class:EventList object.
If the joining EventList (other) is empty a Warning message will appear, and the new EventList will be the same as the starting one.
If the joining EventList (other) has the attribute "time" but the other attributes are empty, such as energy and pi, NaNs will be added to the corresponding times of the other EventList.
If the starting EventList is empty joining acts as filling the events with the attributes of the other EventList.

Waht do you think? Do we need more?

stingray/tests/test_events.py Show resolved Hide resolved
@matteobachetti
Copy link
Member Author

matteobachetti commented Aug 7, 2023

@mgullik I made a slightly different change to the docstring, still along the lines you suggest. Instead of in the return values, I wrote these clarifications in the main text basically. Hope this works

@mgullik
Copy link
Collaborator

mgullik commented Aug 7, 2023

Hi @matteobachetti,
thanks! I approved your pull request.

@matteobachetti matteobachetti added this pull request to the merge queue Aug 18, 2023
Merged via the queue into main with commit f661d45 Aug 18, 2023
13 checks passed
@matteobachetti matteobachetti deleted the improve_eventlist_join branch September 22, 2023 09:36
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

3 participants