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 to_ordinal feature for ordinal regression/classification #17419

Merged
merged 7 commits into from
Jan 24, 2023

Conversation

awsaf49
Copy link
Contributor

@awsaf49 awsaf49 commented Jan 12, 2023

This PR adds the feature to_ordinal for ordinal regression/classification mentioned in issue keras-team/tf-keras#321

@google-cla
Copy link

google-cla bot commented Jan 12, 2023

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.

@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Jan 13, 2023
@google-ml-butler google-ml-butler bot added the keras-team-review-pending Pending review by a Keras team member. label Jan 13, 2023
line to long
@awsaf49
Copy link
Contributor Author

awsaf49 commented Jan 13, 2023

@gbaned please approve the workflows.

Copy link
Contributor

@hertschuh hertschuh left a comment

Choose a reason for hiding this comment

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

Thank you for the Pull Request!
Just a small change:

keras/utils/np_utils.py Show resolved Hide resolved
PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Jan 13, 2023
Copy link
Contributor

@haifeng-jin haifeng-jin 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!
Please also update this file after the PR is merged, by adding to the tf.keras section for new features.

keras/utils/np_utils.py Outdated Show resolved Hide resolved
@haifeng-jin haifeng-jin added kokoro:force-run and removed keras-team-review-pending Pending review by a Keras team member. labels Jan 13, 2023
@haifeng-jin
Copy link
Contributor

@awsaf49 Seems the self.assertTrue() in the test is failing. Would you please fix it? Thanks.

`assertTrue` fails for label shape `(3, 2, 1)` as   ordinal->label creates shape `(3, 2)` hence the mismatch. Using `reshape(label)` with `ordinal` before comparison is a fix.
@google-ml-butler google-ml-butler bot added the keras-team-review-pending Pending review by a Keras team member. label Jan 14, 2023
@awsaf49 awsaf49 requested review from haifeng-jin and hertschuh and removed request for hertschuh and haifeng-jin January 14, 2023 03:42
Copy link
Contributor

@haifeng-jin haifeng-jin left a comment

Choose a reason for hiding this comment

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

LGTM

keras/utils/np_utils.py Outdated Show resolved Hide resolved
@google-ml-butler google-ml-butler bot added kokoro:force-run ready to pull Ready to be merged into the codebase labels Jan 14, 2023
@haifeng-jin haifeng-jin removed the keras-team-review-pending Pending review by a Keras team member. label Jan 14, 2023
@haifeng-jin
Copy link
Contributor

@awsaf49
The tests are still failing.

ERROR: test_to_ordinal (__main__.TestNPUtils)
TestNPUtils.test_to_ordinal
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/kbuilder/.cache/bazel/_bazel_kbuilder/31d6f47147b75c35404d734345be7323/execroot/org_keras/bazel-out/k8-opt/bin/keras/utils/np_utils_test.runfiles/org_keras/keras/utils/np_utils_test.py", line 74, in test_to_ordinal
    self.assertTrue(
  File "/usr/lib/python3.9/unittest/case.py", line 686, in assertTrue
    if not expr:
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

----------------------------------------------------------------------
Ran 3 tests in 0.006s

@haifeng-jin haifeng-jin removed the ready to pull Ready to be merged into the codebase label Jan 14, 2023
@awsaf49
Copy link
Contributor Author

awsaf49 commented Jan 14, 2023

@awsaf49 The tests are still failing.

ERROR: test_to_ordinal (__main__.TestNPUtils)
TestNPUtils.test_to_ordinal
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/kbuilder/.cache/bazel/_bazel_kbuilder/31d6f47147b75c35404d734345be7323/execroot/org_keras/bazel-out/k8-opt/bin/keras/utils/np_utils_test.runfiles/org_keras/keras/utils/np_utils_test.py", line 74, in test_to_ordinal
    self.assertTrue(
  File "/usr/lib/python3.9/unittest/case.py", line 686, in assertTrue
    if not expr:
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

----------------------------------------------------------------------
Ran 3 tests in 0.006s

@haifeng-jin my bad, forgot to put np.all after element-wise comaprision, will fix it in the following commit

ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()
@haifeng-jin haifeng-jin added the ready to pull Ready to be merged into the codebase label Jan 14, 2023
@awsaf49
Copy link
Contributor Author

awsaf49 commented Jan 15, 2023

This PR is stuck due to

1 workflow awaiting approval, First-time contributors need a maintainer to approve running workflows.

@gbaned could you please approve?

@awsaf49
Copy link
Contributor Author

awsaf49 commented Jan 19, 2023

I think this PR looks fine but is on hold for quite some time, It would be really helpful if someone could merge the PR.
cc: @haifeng-jin @hertschuh @gbaned

@haifeng-jin
Copy link
Contributor

@awsaf49 I am doing some required Google internal changes since this PR touches the APIs. Sorry for the delay.

@awsaf49
Copy link
Contributor Author

awsaf49 commented Jan 19, 2023

@haifeng-jin thanks for your effort. I wasn't aware of these internal procedures. Looking forward to hearing from you...

@qlzh727 qlzh727 requested a review from fchollet January 19, 2023 20:51
@google-ml-butler google-ml-butler bot added the keras-team-review-pending Pending review by a Keras team member. label Jan 19, 2023
@haifeng-jin
Copy link
Contributor

@awsaf49 There are more internal review comments that I need to address. So please expect some delays. Thanks.

@haifeng-jin haifeng-jin removed the keras-team-review-pending Pending review by a Keras team member. label Jan 23, 2023
@awsaf49
Copy link
Contributor Author

awsaf49 commented Jan 24, 2023

@awsaf49 There are more internal review comments that I need to address. So please expect some delays. Thanks.

@haifeng-jin Thank you for letting me know.

In the meantime, I would like to propose creating an example on Ordinal Regression, something like "Age Estimation with Ordinal Regression" on the keras.io/examples page. As of now, there is no mention of Ordinal Regression on the call for contribution page. May I proceed with creating a pull request for this example?

cc: @fchollet

@copybara-service copybara-service bot merged commit 77fcb23 into keras-team:master Jan 24, 2023
PR Queue automation moved this from Reviewer Requested Changes to Merged Jan 24, 2023
awsaf49 added a commit to awsaf49/tensorflow that referenced this pull request Jan 24, 2023
For recently added `tf.keras.utils.to_ordinal` utility [here](keras-team/keras#17419)
@awsaf49
Copy link
Contributor Author

awsaf49 commented Jan 24, 2023

@haifeng-jin as the PR has been merged, I've created a PR on TensorFlow to update the RELEASE.md file as you asked, tensorflow/tensorflow/pull/59436

copybara-service bot pushed a commit that referenced this pull request Jan 27, 2023
Imported from GitHub PR #17485

This PR will resolve two issues and add an explanation for `to_ordinal` utility, recently merged in #17419.

It will,
1. Resolve a grammatical error in `Return`
2. Resolve abnormality in api_docs due to a new line in the docstring. [api_docs link](https://www.tensorflow.org/api_docs/python/tf/keras/utils/to_ordinal) <img src="https://user-images.githubusercontent.com/36858976/215018234-4e7b424d-c6df-4baf-89ba-cc561314578f.png" width=300>
3. Add a little explanation for `to_ordinal`

cc: @haifeng-jin
Copybara import of the project:

--
db1ec98 by Awsaf <awsaf49@gmail.com>:

fix grammar

--
bc8929c by Awsaf <awsaf49@gmail.com>:

fix for newline in api_docs]

new line creates abnormality in api_docs in https://www.tensorflow.org/api_docs/python/tf/keras/utils/to_ordinal
--
3ab1d2e by Awsaf <awsaf49@gmail.com>:

add little explanation

Merging this change closes #17485

FUTURE_COPYBARA_INTEGRATE_REVIEW=#17485 from awsaf49:to_ordinal 3ab1d2e
PiperOrigin-RevId: 505141352
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to pull Ready to be merged into the codebase size:M
Projects
PR Queue
Merged
Development

Successfully merging this pull request may close these issues.

None yet

5 participants