349: Implemented round() function #359

VidhyaVarshanyJS · 2024-01-30T08:54:00Z

I have made changes the files

Kindly review my implementation.

google-cla · 2024-01-30T08:54:04Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

ianspektor

This is a great start! 🚀

Missing:

Tests
- Be thorough
- Test edge cases
- Test passing an EventSet with a single feature
- Test passing an EventSet with several features
- Test passing an EventSet with features of non-accepted types (e.g. string, ints)
- Test passing an EventSet with some float32 and some float64 features, ensure output types are the same as input ones
Adding the new operation to the docs

See last two points in https://temporian.readthedocs.io/en/latest/contributing/#developing-a-new-operator for guidance and examples.

Let's keep this PR for the round() function only, and open a new one for floor() and ceil() once done.

Thanks!

temporian/core/event_set_ops.py

ianspektor · 2024-01-30T14:38:12Z

temporian/core/operators/unary.py

+    @classmethod
+    def allowed_dtypes(cls) -> List[DType]:
+        return [
+            DType.INT32,
+            DType.INT64,
+        ]
+
+    @classmethod
+    def get_output_dtype(cls, feature_dtype: DType) -> DType:
+        return feature_dtype


These look off:

allowed_dtypes specifies the types that the operator can consume - should be DType.FLOAT32 and DType.FLOAT64

get_output_dtype returns the output dtype, given the input dtype - in this case it should return the same type as received (I said it should return ints in the issue, edited it, needs to return floats because they have a much larger range than ints)

ianspektor · 2024-01-30T14:38:51Z

temporian/core/operators/unary.py

 operator_lib.register_operator(AbsOperator)
 operator_lib.register_operator(LogOperator)
+operator_lib.register_operator(RoundOperator)
+


extra newline here too

VidhyaVarshanyJS · 2024-01-30T16:39:13Z

This is a great start! 🚀

Missing:

Tests

Be thorough

Test edge cases

Test passing an EventSet with a single feature

Test passing an EventSet with several features

Test passing an EventSet with features of non-accepted types (e.g. string, ints)

Test passing an EventSet with some float32 and some float64 features, ensure output types are the same as input ones

Adding the new operation to the docs

See last two points in https://temporian.readthedocs.io/en/latest/contributing/#developing-a-new-operator for guidance and examples.

Let's keep this PR for the round() function only, and open a new one for floor() and ceil() once done.

Thanks!

Thanks for your time!
Shall I create a separate test_round.py or implement test cases within the test_unary.py itself?

ianspektor · 2024-01-30T17:10:05Z

Shall I create a separate test_round.py or implement test cases within the test_unary.py itself?

Let's do test_unary.py, since they're implemented in the same file too.

VidhyaVarshanyJS · 2024-02-03T04:24:48Z

Hi..
I have added the test cases for the round() in test_unary.py for the all the above combinations that is mentioned.
Kindly review my changes.
Thanks

ianspektor · 2024-02-05T15:02:38Z

temporian/core/operators/test/test_unary.py

+        with self.assertRaises(TypeError):
+            _ = evset


missing calling round() on the evset here - this shouldn't be raising anything? did you manage to run the tests locally to ensure they pass?

ianspektor · 2024-02-05T15:06:02Z

temporian/core/operators/test/test_unary.py

+        )
+        expected = event_set(
+            timestamps=[1, 2],
+            features={"a": [11, 12], "b": [1, 3]},


I don't think these will pass, since a list of ints will yield an eventset with int features, which won't be equal to the float results of round(). Ensure your new tests are passing by running bazel test //temporian/core/operators/test:test_unary --config=macos --test_output=errors or bazel test //temporian/core/operators/test:test_unary --config=linux --test_output=errors depending on your OS

ianspektor · 2024-02-05T15:07:23Z

temporian/core/operators/test/test_unary.py

+    def test_round_float32_and_float64_features(self):
+        evset = event_set(
+            timestamps=[1, 2],
+            features={"a": [10.5, 11.7], "b": [1.2, 2.9]},
+        )
+        expected = event_set(
+            timestamps=[1, 2],
+            features={"a": [11.0, 12.0], "b": [1.0, 3.0]},
+            same_sampling_as=evset,
+        )


which of these are being defined as f32 and which as f64? see the f64 and f32 methods in temporian/test/utils.py to explicitly create an eventset with the desired feature types

I need a clarification. Can I use the numpy for specifying the float32 and float64 separately as

float32

def test_round_float32(self): evset = event_set( timestamps=[1, 2], features={"a": np.array([10.5, 11.7], dtype=np.float32), "b": np.array([1.2, 2.9], dtype=np.float32)}, ) expected = event_set( timestamps=[1, 2], features={"a": np.array([11.0, 12.0], dtype=np.float32), "b": np.array([1.0, 3.0], dtype=np.float32)}, same_sampling_as=evset, ) assertOperatorResult(self, evset.round(), expected) assertOperatorResult(self, round(evset), expected) # __round__ magic

float64

def test_round_float64(self): evset = event_set( timestamps=[1, 2], features={"a": np.array([10.5, 11.7], dtype=np.float64), "b": np.array([1.2, 2.9], dtype=np.float64)}, ) expected = event_set( timestamps=[1, 2], features={"a": np.array([11.0, 12.0], dtype=np.float64), "b": np.array([1.0, 3.0], dtype=np.float64)}, same_sampling_as=evset, ) assertOperatorResult(self, evset.round(), expected) assertOperatorResult(self, round(evset), expected) # __round__ magic

ianspektor · 2024-02-05T15:07:45Z

temporian/core/operators/unary.py

+            DType.INT32,
+            DType.INT64,


ints shouldn't be allowed

ianspektor

Good work so far, please run tests locally to make sure they pass before submitting

ianspektor · 2024-02-05T15:28:55Z

temporian/core/event_set_ops.py

+        self: EventSetOrNode,
+    ) -> EventSetOrNode:
+        """Rounds the values of an [`EventSet`][temporian.EventSet]'s features to the nearest integer.
+


Add another line specifying that only float types are allowed, and the output type will always be the same as the input's

temporian/core/event_set_ops.py

ianspektor · 2024-02-05T15:34:25Z

One more thing, please merge main branch into your PR's branch so that tests are ran on it (fixed in #363)

javiber

Looking good, There are some leftovers from the merge on index.md
Black should take care of the incorrect indentations and unnecessary empty lines. If you have any issue running black reach out to me on discord

javiber · 2024-03-06T13:45:51Z

temporian/core/operators/test/test_unary.py

+        assertOperatorResult(self, evset.round(), expected)
+        assertOperatorResult(self, round(evset), expected)  # __round__ magic
+
+     def test_correct_sin(self) -> None:


The indentation of this line is off. Also no need for type annotations in these test

Suggested change

def test_correct_sin(self) -> None:

def test_correct_sin(self):

You should run black . on the root of the repository (might need to call poetry shell or pre-ped poetry run) to fix the other formatting issues

javiber · 2024-03-06T13:48:24Z

docs/src/reference/index.md

 | ---------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------- |
 | [`tp.combine()`][temporian.combine]                                                                        | Combines events from [`EventSets`][temporian.EventSet] with different samplings.                               |
 | [`tp.glue()`][temporian.glue]                                                                              | Concatenates features from [`EventSets`][temporian.EventSet] with the same sampling.                           |
+| [`EventSet.abs()`][temporian.EventSet.abs]                                                                 | Computes the absolute value of the features.


abs is repeated on line 49 below

javiber · 2024-03-06T13:48:50Z

docs/src/reference/index.md

 | [`tp.combine()`][temporian.combine]                                                                        | Combines events from [`EventSets`][temporian.EventSet] with different samplings.                               |
 | [`tp.glue()`][temporian.glue]                                                                              | Concatenates features from [`EventSets`][temporian.EventSet] with the same sampling.                           |
+| [`EventSet.abs()`][temporian.EventSet.abs]                                                                 | Computes the absolute value of the features.
+| [`EventSet.add_index()`][temporian.EventSet.add_index]                                                     | Adds indexes to an [`EventSet`][temporian.EventSet].                                                           |


add_index is also repeated below

javiber · 2024-03-06T13:50:21Z

temporian/core/event_set_ops.py

+        self: EventSetOrNode,
+    ) -> EventSetOrNode:
+        """Rounds the values of an [`EventSet`][temporian.EventSet]'s features to the nearest integer.
+only float types are allowed and the output. Output type wil be same as the input type


indentation is incorrect, black should fix this

javiber · 2024-03-06T13:57:40Z

temporian/core/operators/test/test_unary.py

+        assertOperatorResult(self, evset.round(), expected)
+        assertOperatorResult(self, round(evset), expected)  # __round__ magic
+
+    def test_round_float64(self):


test_round_float64 and test_round_float32 are doing the same thing. You can use functions f64 and f32 to create an array with that type (link).
f64 is used above in test_correct_isnan

javiber · 2024-03-06T13:58:37Z

temporian/core/operators/unary.py

    def op_key_definition(cls) -> str:
        return "ARCTAN"

+


unnecessary empty line

javiber · 2024-03-06T13:58:42Z

temporian/core/operators/unary.py

    assert isinstance(input, EventSetNode)

    return ArcTanOperator(
+


unnecessary empty line

implemented round() function

9e1affb

ianspektor reviewed Jan 30, 2024

View reviewed changes

added test cases for round in test_unary.py

b64b224

ianspektor approved these changes Feb 5, 2024

View reviewed changes

ianspektor requested changes Feb 5, 2024

View reviewed changes

ianspektor reviewed Feb 5, 2024

View reviewed changes

made changes in few files

94ab060

javiber changed the title ~~implemented round() function~~ 349: Implemented round() function Mar 4, 2024

Merge branch 'main' into vidhya

f357b6b

javiber requested changes Mar 6, 2024

View reviewed changes

	def test_correct_sin(self) -> None:
	def test_correct_sin(self):

		assert isinstance(input, EventSetNode)

		return ArcTanOperator(

349: Implemented round() function #359

Are you sure you want to change the base?

349: Implemented round() function #359

Uh oh!

Conversation

VidhyaVarshanyJS commented Jan 30, 2024

Uh oh!

google-cla bot commented Jan 30, 2024

Uh oh!

ianspektor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VidhyaVarshanyJS commented Jan 30, 2024

Uh oh!

ianspektor commented Jan 30, 2024

Uh oh!

VidhyaVarshanyJS commented Feb 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianspektor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ianspektor commented Feb 5, 2024

Uh oh!

javiber left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants