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.

Sign up for GitHub

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

Jump to bottom

doc improvement for tf.math.asin and tf.math.atan #26494

Merged
merged 3 commits into from
Mar 20, 2019
Merged

doc improvement for tf.math.asin and tf.math.atan #26494

merged 3 commits into from
Mar 20, 2019

Conversation

Sudeepam97
Copy link
Contributor

PR for issue #26492

@rthadur rthadur requested a review from martinwicke March 8, 2019 21:45
@rthadur rthadur self-assigned this Mar 8, 2019
@rthadur rthadur added this to Assigned Reviewer in PR Queue via automation Mar 8, 2019
@rthadur rthadur added size:S CL Change Size: Small awaiting review Pull request awaiting review labels Mar 8, 2019
@dksb dksb removed this from Assigned Reviewer in PR Queue Mar 8, 2019
martinwicke
martinwicke requested changes Mar 11, 2019
View reviewed changes
Copy link
Member

@martinwicke martinwicke left a comment

Choose a reason for hiding this comment

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

Do we not have acos?

Assuming we do, the same change needs to be made there as well I assume.

tensorflow/core/api_def/base_api/api_def_Asin.pbtxt Show resolved Hide resolved
tensorflow/core/api_def/base_api/api_def_Atan.pbtxt Show resolved Hide resolved
@Sudeepam97
Copy link
Contributor Author

Do we not have acos?

Assuming we do, the same change needs to be made there as well I assume.

Actually, I have fixed acos in #26017. Would you be able to look at that PR as well? It has been pending for a long time.

As for the changes, will writing something like...
"Note: The output of tf.math.asin will lie within the invertible range of sine i.e. [-pi/2, pi/2] "
...be enough or should I add another usage example that demonstrates this?

@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Mar 12, 2019
@martinwicke
Copy link
Member

I think the wording changes you propose are good.

This comment also applies to #26017, can you make the same change to cos (except of course the range is different)?

@Sudeepam97
Copy link
Contributor Author

Alright, I've specified the range for all the three operations.

@rthadur rthadur added this to Assigned Reviewer in PR Queue via automation Mar 20, 2019
@rthadur rthadur moved this from Assigned Reviewer to Approved by Reviewer in PR Queue Mar 20, 2019
@rthadur rthadur added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 20, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 20, 2019
@tensorflow-copybara tensorflow-copybara merged commit fca1efc into tensorflow:master Mar 20, 2019
PR Queue automation moved this from Approved by Reviewer to Merged Mar 20, 2019
tensorflow-copybara pushed a commit that referenced this pull request Mar 20, 2019
@tensorflower-gardener
Merge pull request #26494 from Sudeepam97:doc_patch_2
e9ad5b5 
PiperOrigin-RevId: 239497933
@Sudeepam97
Copy link
Contributor Author

@martinwicke will you be able to approve #26017 as well if that PR is fine?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinwicke martinwicke martinwicke approved these changes

Assignees

@rthadur rthadur

Labels
cla: yes ready to pull PR ready for merge process size:S CL Change Size: Small
Projects
PR Queue
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Sudeepam97 @martinwicke @googlebot @kokoro-team @tensorflowbutler @tensorflow-copybara @rthadur