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

IntervalSet creation from (start, end) pairs #304

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

eschombu
Copy link
Contributor

@eschombu eschombu commented Jun 26, 2024

It's a bit counter-intuitive sometimes to split up a sequence of intervals into the starts and ends, and easier sometimes to write a sequence of start-end pairs. This change allows the IntervalSet to accept such a sequence of pairs, or a single pair.

Example:

single_intvl = IntervalSet((0, 60))
several_intvls = IntervalSet([(0, 5), (5, 10), (40, 50)])

I also added the capability to pass a variable that can be used for IntervalSet creation to the time_support parameter during TsGroup initialization. Meaning, if time_support is not an IntervalSet instance, time_support = IntervalSet(time_support) will be attempted.

I added tests and updated the doc string. This is my first PR to the repo, so more than happy for feedback on:

  • whether this sort of behavior is desired; my tendency might be toward more flexibility than other authors prefer
  • code style and practices
  • PR style and practices
  • anything else!

@eschombu eschombu requested a review from gviejo as a code owner June 26, 2024 22:45
@gviejo gviejo changed the base branch from main to dev July 5, 2024 08:45
@@ -94,7 +94,7 @@ def __init__(self, start, end=None, time_units="s", **kwargs):

Parameters
----------
start : numpy.ndarray or number or pandas.DataFrame or pandas.Series
start : numpy.ndarray or number or pandas.DataFrame or pandas.Series or iterable of (start, end) pairs
Beginning of intervals
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add more description here.

@gviejo
Copy link
Contributor

gviejo commented Jul 5, 2024

I agree for the flexibility and I think the new argument for IntervalSet makes sense. For TsGroup, I would not do it as I can see it being hard to maintain on the long run. In this case, forcing to pass an IntervalSet is probably easier.

try:
start_end_array = np.array(list(start))
if start_end_array.ndim == 1:
start, end = start_end_array
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure I understand this. In this case you need to have start_end_array being of size 2 but you check for ndim?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was allowing the case of ValueError raised by the ndim == 1 case with size != 2 to be handled by the try...except block, but I think you're right that this wasn't the clearest and best way to handle this. Hopefully the modified version is clearer.

@@ -90,6 +90,15 @@ def test_create_ts_group_with_time_support(self, group):
assert np.all(first >= ep[0, 0])
assert np.all(last <= ep[0, 1])

def test_create_ts_group_with_time_support_tuple(self, group):
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove this one

@@ -124,6 +124,14 @@ def __init__(

self._metadata = pd.DataFrame(index=self.index, columns=["rate"], dtype="float")

# If time_support is passed, all elements of data are restricted prior to init
Copy link
Contributor

Choose a reason for hiding this comment

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

I would force IntervalSet here.

@eschombu
Copy link
Contributor Author

eschombu commented Aug 9, 2024

Ok, I took out the portion where non-IntervalSet values to TsGroup time_support were converted. I tried to make the IntervalSet modifications clearer and more robust, let me know what you think.

@gviejo
Copy link
Contributor

gviejo commented Aug 13, 2024

Thank you Eric

@gviejo gviejo merged commit e3bf7cc into pynapple-org:dev Aug 13, 2024
11 checks passed
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