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 MaxConsecutiveFalse, MaxConsecutiveNegatives, MaxConsecutivePositives, MaxConsecutiveTrue, MaxConsecutiveZeros, NumConsecutiveGreaterMean, NumConsecutiveLessMean #2420

Merged
merged 17 commits into from
Dec 20, 2022

Conversation

gsheni
Copy link
Contributor

@gsheni gsheni commented Dec 20, 2022

No description provided.

@gsheni gsheni self-assigned this Dec 20, 2022
@gsheni gsheni marked this pull request as ready for review December 20, 2022 16:19
@codecov
Copy link

codecov bot commented Dec 20, 2022

Codecov Report

Merging #2420 (62f6011) into main (b3b3a9e) will increase coverage by 0.00%.
The diff coverage is 99.54%.

@@           Coverage Diff            @@
##             main    #2420    +/-   ##
========================================
  Coverage   99.44%   99.44%            
========================================
  Files         331      340     +9     
  Lines       20432    20870   +438     
========================================
+ Hits        20319    20755   +436     
- Misses        113      115     +2     
Impacted Files Coverage Δ
...etools/tests/entityset_tests/test_serialization.py 100.00% <ø> (ø)
...tests/primitive_tests/test_expanding_primitives.py 100.00% <ø> (ø)
...andard/aggregation/num_consecutive_greater_mean.py 96.96% <96.96%> (ø)
.../standard/aggregation/num_consecutive_less_mean.py 96.96% <96.96%> (ø)
...etools/primitives/standard/aggregation/__init__.py 100.00% <100.00%> (ø)
...ives/standard/aggregation/max_consecutive_false.py 100.00% <100.00%> (ø)
.../standard/aggregation/max_consecutive_negatives.py 100.00% <100.00%> (ø)
.../standard/aggregation/max_consecutive_positives.py 100.00% <100.00%> (ø)
...tives/standard/aggregation/max_consecutive_true.py 100.00% <100.00%> (ø)
...ives/standard/aggregation/max_consecutive_zeros.py 100.00% <100.00%> (ø)
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@gsheni gsheni enabled auto-merge (squash) December 20, 2022 18:52
Copy link
Contributor

@thehomebrewnerd thehomebrewnerd left a comment

Choose a reason for hiding this comment

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

Looks like we have two new uncovered lines with these updates. Might be nice to add a test to cover those if it's not a big lift.

primitive_instance = self.primitive()
def test_regular(self, dtype):
if dtype == "int64":
pytest.skip("floats not supported in int64")
Copy link
Contributor

Choose a reason for hiding this comment

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

floats -> nans

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this was intentional message. I made a better one
a014517

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see. I didn't follow what was being tested closely enough.

primitive_instance = self.primitive()
def test_regular(self, dtype):
if dtype == "int64":
pytest.skip("floats not supported in int64")
Copy link
Contributor

Choose a reason for hiding this comment

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

floats -> nans

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this was intentional message. I made a better one
a014517

primitive_instance = self.primitive()
def test_regular(self, dtype):
if dtype == "int64":
pytest.skip("floats not supported in int64")
Copy link
Contributor

Choose a reason for hiding this comment

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

floats -> nans

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this was intentional message. I made a better one
a014517

primitive_instance = self.primitive()
def test_all_float(self, dtype):
if dtype == "int64":
pytest.skip("floats not supported in int64")
Copy link
Contributor

Choose a reason for hiding this comment

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

floats -> nans

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this was intentional message. I made a better one
a014517

Copy link
Contributor

@thehomebrewnerd thehomebrewnerd left a comment

Choose a reason for hiding this comment

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

Small nitpicks, but not blocking

@@ -69,7 +69,7 @@ def test_all_int(self, dtype):

def test_all_float(self, dtype):
if dtype == "int64":
pytest.skip("floats not supported in int64")
pytest.skip("test array contains floats which are not supported int64")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: the floats used in this test could all be represented as integers. Should we change?

@@ -126,7 +126,7 @@ def test_all_int(self, dtype):

def test_all_float(self, dtype):
if dtype == "int64":
pytest.skip("floats not supported in int64")
pytest.skip("test array contains floats which are not supported int64")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: the floats used in this test could all be represented as integers. Should we change?

@gsheni gsheni merged commit 4ce3c2a into main Dec 20, 2022
@gsheni gsheni deleted the max_conse_prims branch December 20, 2022 20:45
@thehomebrewnerd thehomebrewnerd mentioned this pull request Jan 5, 2023
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