[GCI] Standardised Edge Detect module code comments #1346
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5 tasks
Codecov Report
@@ Coverage Diff @@
## main #1346 +/- ##
=======================================
Coverage 66.12% 66.12%
=======================================
Files 125 125
Lines 2571 2571
Branches 404 404
=======================================
Hits 1700 1700
Misses 871 871
|
SidharthBansal
requested changes
Dec 9, 2019
SidharthBansal
requested changes
Dec 10, 2019
|
Hmm. I am not very good at English. I asked one the dev of a library who
has experience in development field and also happens to be very good ar
literature said that any group of words can be considered a phrase and the
full stop makes it a sentence. That is an infinite loop so I think making
it a phrase would be the best option. He said that this one is both a
sentence and a phrase which blew my mind.
…On Thu, 12 Dec, 2019, 9:57 PM Sidharth Bansal, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/modules/EdgeDetect/EdgeUtils.js
<#1346 (comment)>
:
> @@ -21,7 +23,7 @@ module.exports = function(pixels, highThresholdRatio, lowThresholdRatio, useHyst
grads.push([]);
angles.push([]);
for (var y = 0; y < pixels.shape[1]; y++) {
- var result = sobelFilter(
+ var result = sobelFilter( // Convolves the sobel filter on every pixel.
I supposed that a sentence consists of a subject and predicate. Here only
predicate is present, hence I thought that it is a phrase.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1346?email_source=notifications&email_token=AIJI5HZBNUKKL7BP7T44LVTQYJQ7PA5CNFSM4JYKOZBKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCPAB7VY#discussion_r357242716>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIJI5H5XG57GHRWDZ7EB27LQYJQ7PANCNFSM4JYKOZBA>
.
|
|
It is a debatable thing. Most developers don't use a full stop in getter and setter things. As they are in a hurry. So, I asked to remove unnecessary full stops. This will others to not write full stops in every single comment. |
SidharthBansal
approved these changes
Dec 15, 2019
SidharthBansal
approved these changes
Dec 15, 2019
|
Looking at this one now! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1341 (<=== Replace
0000with the Issue Number)Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
npm test@publiclab/is-reviewersfor help, in a comment belowIf tests do fail, click on the red
Xto learn why by reading the logs.Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
Please make sure to get at least two reviews before asking for merging the PR as that would make the PR more reliable on our part
Thanks!