Skip to content
This repository has been archived by the owner on Aug 15, 2019. It is now read-only.

Update LRN docs to reflect removal of 'normRegion' argument #1073

Merged
merged 4 commits into from
Jun 6, 2018

Conversation

jgartman
Copy link
Contributor

@jgartman jgartman commented Jun 2, 2018

Description

The documentation for the radius parameter of localResponseNormalization mentions an argument that has been removed from the method.


For repository owners only:

Please remember to apply all applicable tags to your pull request.
Tags: FEATURE, BREAKING, BUG, PERF, DEV, DOC, SECURITY

For more info see: /~https://github.com/tensorflow/tfjs/blob/master/DEVELOPMENT.md


This change is Reviewable

@jgartman
Copy link
Contributor Author

jgartman commented Jun 3, 2018

On second thought, maybe it should be:

The number of adjacent channels in the 1D normalization window. In Tensorflow this param is called depth_radius.

@nsthorat
Copy link
Contributor

nsthorat commented Jun 5, 2018

Thanks a lot Josh!


Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.


src/ops/lrn.ts, line 33 at r1 (raw file):

   *     normalized independently.
   * @param radius The number of adjacent channels or spatial locations of the
   *     1D normalization window. In Tensorflow this param is called

Actually, can you just remove the second sentence and rename "radius" to "depth_radius" in the code below and then we'll align with TF python?


Comments from Reviewable

@jgartman
Copy link
Contributor Author

jgartman commented Jun 6, 2018

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


src/ops/lrn.ts, line 33 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

Actually, can you just remove the second sentence and rename "radius" to "depth_radius" in the code below and then we'll align with TF python?

Done.


Comments from Reviewable

@nsthorat
Copy link
Contributor

nsthorat commented Jun 6, 2018

:lgtm_strong:


Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@nsthorat
Copy link
Contributor

nsthorat commented Jun 6, 2018

Thanks Josh!

@nsthorat nsthorat merged commit abcd97b into tensorflow:master Jun 6, 2018
@jgartman jgartman deleted the lrnDocs branch August 25, 2018 17:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants