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

added erf op to math.py #908

Closed
wants to merge 14 commits into from
Closed

added erf op to math.py #908

wants to merge 14 commits into from

Conversation

sqali
Copy link
Contributor

@sqali sqali commented Sep 18, 2023

Hi @fchollet ,

Hope you are doing well. As discussed in issue keras-team/keras#18442, I have raised this PR to work on the ERF function. Kindly review the changes I have currently made. Once approved, I will go ahead with the implementation for JAX, torch, and numpy backends.

Thanks & Regards

Copy link
Member

@fchollet fchollet left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! 👍


def erf(x):
if not isinstance(x, torch.Tensor):
x = torch.tensor(x)
Copy link
Member

Choose a reason for hiding this comment

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

You can just do x = convert_to_tensor(x) unconditionally

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

"""Computes the error function of x element-wise.

Args:
input_tensor: A tensor of type `float32` or `float64`.
Copy link
Member

Choose a reason for hiding this comment

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

It can have more types, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Edited the comments based on that


@keras_core_export("keras_core.ops.erf")
def erf(x):
"""Functional interface to the `Erf` operation."""
Copy link
Member

Choose a reason for hiding this comment

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

This is where the docstring should be, not the op above, since this is the public symbol.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Relocated it!

>>> y_large = Erf()(x_large)
"""

def __init__(self):
Copy link
Member

Choose a reason for hiding this comment

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

Not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it

def compute_output_spec(self, input_tensor):
return KerasTensor(shape=input_tensor.shape, dtype=input_tensor.dtype)

def call(self, input_tensor):
Copy link
Member

Choose a reason for hiding this comment

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

Just x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced with x

keras_core/ops/math_test.py Outdated Show resolved Hide resolved
Copy link
Member

@fchollet fchollet left a comment

Choose a reason for hiding this comment

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

Thanks for the updates!

return KerasTensor(shape=x.shape, dtype=x.dtype)

def call(self, x):
return backend.erf(x)
Copy link
Member

Choose a reason for hiding this comment

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

You need to call backend.math. Tests are failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

implemented it


Examples:

# Basic usage
Copy link
Member

Choose a reason for hiding this comment

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

Since you're not printing any outputs, just use a fenced code block for the code example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@sqali
Copy link
Contributor Author

sqali commented Sep 21, 2023

Hi @fchollet ,

Thanks so much for the guidance. The Tests are failing because the errors are greater than the tolerance level (1×10^-5) set as you can see below.

Metric Value
Max absolute difference 0.12837633
Max relative difference 0.12837917

Kindly guide me on how to resolve this issue.

Thanks & Regards

@fchollet
Copy link
Member

You can just lower the precision of this particular check to 1e-4.

lowered the precision tolerance to 1e-4 for erf function
@sqali
Copy link
Contributor Author

sqali commented Sep 22, 2023

Hi @fchollet ,

Thanks for the reply, Lowered the precision to 1e-4 such that it would resolve the issue but the maximum difference is around 0.12 which is way higher than 0.0001 which still leads to failing tests. Kindly advise.


def test_erf_operation_edge_cases(self):
# Test for edge cases
edge_values = np.array([1e10, -1e10, 1e-10, -1e-10], dtype=np.float64)
Copy link
Member

Choose a reason for hiding this comment

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

Your test values are too large. Try 1e5. This the source of the large discrepancy IMO.

Copy link
Contributor Author

@sqali sqali Sep 22, 2023

Choose a reason for hiding this comment

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

I have implemented the changes, but I can see from the tests that it is failing for the below array examples. I wonder if there is anything wrong in the implementation function itself.

  • x: array([ 1.128379e+00, -1.128379e+00, 1.273240e-05, -1.273240e-05])
  • x: array([-1.128354, -1.123101, -0.950886, 0. , 0.950886, 1.123101, 1.128354])

image

lowered the test values to 1e5
@fchollet
Copy link
Member

Keras Core is becoming Keras 3, and we're switching development to the main repository! Please reopen this PR in the keras-team/keras repository. Unfortunately we aren't able to automatically transfer PRs (but we have transferred all issues).

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