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

[Community Tutorials] Studying Land Surface Temperature in Uganda #51

Merged
merged 25 commits into from Oct 22, 2019

Conversation

hiyer09
Copy link
Contributor

@hiyer09 hiyer09 commented Oct 2, 2019

This is a tutorial to describe land surface temperature using data from Uganda. I initially submitted with another tutorial (patch-1) but closed that review request.

@hiyer09 hiyer09 changed the title Create index.md [Community Tutorials][Describing Land Surface Temperature in Uganda] Oct 2, 2019
@gino-m gino-m changed the title [Community Tutorials][Describing Land Surface Temperature in Uganda] [Community Tutorials] Studying Land Surface Temperature in Uganda Oct 4, 2019
@gino-m gino-m requested review from gino-m and schwehr October 4, 2019 17:01
@gino-m
Copy link
Collaborator

gino-m commented Oct 4, 2019

Thanks for resubmitting this in a separate PR! Assigning for review:

schwehr
schwehr previously requested changes Oct 4, 2019
Copy link
Contributor

@schwehr schwehr 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 pull request!

Some comments to get the review process started:

Apologies that some of my comments are brief. If @gino-m has comments, please take his preferences over mine.

  • Remove all trailing whitespace from the md and code. e.g. perl -pi -e 's/\s+\n/\n/g' index.md
  • Please include the JavaScript code so I can comment directly on it. Comments from the code:
    • The code should be able to stand alone from the doc. It just needs a bit more work to do that
    • For the first usage: LST → Land Surface Temperature (LST)
    • For comments, use consistent capitalization, spacing.
    • Please convert the fusion tables asset to an Earth Engine asset or use an existing public asset
    • The filterdate comment says 1 year, so using date.advance(1, 'year') would be more obvious
    • For the modLSTday.map function:
      • Please break the steps up and add some comments.
      • Magic numbers are hard to follow. What is the 0.02? What should I look for in https://en.wikipedia.org/wiki/Kelvin ?
      • Comment on why the copyProperties
    • Please explain why this needs to take a mean of system:time_start in ui.Chart.image.series
    • Please cleanup the palette. Use ['name1', 'name2', ...] . I prefer color names if at all possible. Names can be had here:
      https://gist.github.com/schwehr/3d72e09263e2341167b12f619ca5d4b2

tutorials/ph-ug-temp/index.md Show resolved Hide resolved
tutorials/ph-ug-temp/index.md Outdated Show resolved Hide resolved
tutorials/ph-ug-temp/index.md Outdated Show resolved Hide resolved
tutorials/ph-ug-temp/index.md Outdated Show resolved Hide resolved
tutorials/ph-ug-temp/index.md Outdated Show resolved Hide resolved
tutorials/ph-ug-temp/index.md Outdated Show resolved Hide resolved
tutorials/ph-ug-temp/index.md Outdated Show resolved Hide resolved
tutorials/ph-ug-temp/index.md Outdated Show resolved Hide resolved
tutorials/ph-ug-temp/index.md Outdated Show resolved Hide resolved
tutorials/ph-ug-temp/index.md Outdated Show resolved Hide resolved
@gino-m
Copy link
Collaborator

gino-m commented Oct 10, 2019

@schwehr Thanks for reviewing. When ready please resolve all outstanding comments and assign back to me.

@hiyer09 hiyer09 requested a review from schwehr October 12, 2019 12:28
@hiyer09
Copy link
Contributor Author

hiyer09 commented Oct 12, 2019

@schwehr Thanks for reviewing. When ready please resolve all outstanding comments and assign back to me.

Hi Gino, Kurt, thanks for your message. I responded to Kurt's comments last week and thought I requested Gino to re-review but maybe it didn't work. I tried to figure out how to assign back to Gino, but it looks like I don't have permission (the option with checkboxes and assigning doesn't appear for me).

Hari

@jdbcode jdbcode assigned gino-m and unassigned schwehr Oct 15, 2019
@jdbcode
Copy link
Member

jdbcode commented Oct 15, 2019

Hello @hiyer09 - thanks for addressing the comments and for your patience with the review process. I just assigned back to @gino-m.

@schwehr
Copy link
Contributor

schwehr commented Oct 15, 2019

Hi @hiyer09, Apologies for the slow review. I was out sick most of last week.

gino-m
gino-m previously requested changes Oct 16, 2019
Copy link
Collaborator

@gino-m gino-m left a comment

Choose a reason for hiding this comment

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

My apologies for the delay and the back and forth - I'm really looking forward to publishing this tutorial, I think it will help a lot of EE users!

I've added a few editorial nits here and there but overall looks great. Please "Resolve" all comments once ready and ping the thread and we can prepare for final publication. Thanks again and thanks @schwehr for the review!

tutorials/ph-ug-temp/index.md Outdated Show resolved Hide resolved
tutorials/ph-ug-temp/index.md Outdated Show resolved Hide resolved
tutorials/ph-ug-temp/index.md Outdated Show resolved Hide resolved
tutorials/ph-ug-temp/index.md Outdated Show resolved Hide resolved
tutorials/ph-ug-temp/index.md Outdated Show resolved Hide resolved
tutorials/ph-ug-temp/index.md Outdated Show resolved Hide resolved
tutorials/ph-ug-temp/index.md Outdated Show resolved Hide resolved
tutorials/ph-ug-temp/index.md Outdated Show resolved Hide resolved
tutorials/ph-ug-temp/index.md Outdated Show resolved Hide resolved
tutorials/ph-ug-temp/ph-ug-temp.js.txt Outdated Show resolved Hide resolved
jdbcode
jdbcode previously approved these changes Oct 22, 2019
Copy link
Member

@jdbcode jdbcode left a comment

Choose a reason for hiding this comment

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

@hiyer09 Thanks for your edits! I made editorial changes. Unless you object to any, I'll merge this PR and get it published to the Developer Guide.

Please let me know if you approve of the changes.

@jdbcode jdbcode dismissed stale reviews from gino-m and schwehr October 22, 2019 17:12

Changes have been addressed by @hiyer09 and @jdbcode

@hiyer09
Copy link
Contributor Author

hiyer09 commented Oct 22, 2019

@hiyer09 Thanks for your edits! I made editorial changes. Unless you object to any, I'll merge this PR and get it published to the Developer Guide.

Please let me know if you approve of the changes.

@jdbcode thanks so much for looking through the code, your edits look great from my end.

@gino-m
Copy link
Collaborator

gino-m commented Oct 22, 2019

@hiyer09 Out of curiosity, are you editing these files via the GitHub Desktop client for Windows? There are some Windows-based line endings in the .js file that I expected git to convert. Nothing to worry about, just want to make sure we have our config set up correctly. Thanks!

@hiyer09
Copy link
Contributor Author

hiyer09 commented Oct 22, 2019

@hiyer09 Out of curiosity, are you editing these files via the GitHub Desktop client for Windows? There are some Windows-based line endings in the .js file that I expected git to convert. Nothing to worry about, just want to make sure we have our config set up correctly. Thanks!

@gino-m Hi Gino, I am editing the markdown file on Github's web interface in chrome. For the javascript file I copy/pasted code from Earth Engine into notepad in windows 10, and then changed the extension on Github. Is there a better way to manage the javascript code?

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes CLAs approved labels Oct 22, 2019
@hiyer09
Copy link
Contributor Author

hiyer09 commented Oct 22, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot I consent.

@jdbcode
Copy link
Member

jdbcode commented Oct 22, 2019

@googlebot I consent

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes CLAs approved and removed cla: no labels Oct 22, 2019
@jdbcode jdbcode merged commit 1d378a5 into google:master Oct 22, 2019
@jdbcode
Copy link
Member

jdbcode commented Oct 22, 2019

@hiyer09 - congratulations - your tutorial has been merged! It should be published tomorrow. I'll ping you here with the URL when it is up.

Do you have a Twitter account? I'd like to announce this tutorial in a tweet and tag you. If you'd prefer it be more anonymous, I'll not tag you. Please let me know.

Well done!

@hiyer09
Copy link
Contributor Author

hiyer09 commented Oct 22, 2019 via email

@jdbcode
Copy link
Member

jdbcode commented Oct 23, 2019

@hiyer09
Copy link
Contributor Author

hiyer09 commented Oct 23, 2019 via email

@gino-m
Copy link
Collaborator

gino-m commented Oct 24, 2019

@hiyer09 - your tutorial is live! 🎉
https://developers.google.com/earth-engine/tutorials/community/ph-ug-temp

Congratulations Hari, beautiful work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes CLAs approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants