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

PSPNet Guide #547

Merged
merged 15 commits into from
Dec 31, 2019
Merged

PSPNet Guide #547

merged 15 commits into from
Dec 31, 2019

Conversation

divyanshj16
Copy link
Contributor

Adds guide for how PSPNet works and how to initialize it inside arcgis.learn.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB.

@divyanshj16
Copy link
Contributor Author

divyanshj16 commented Nov 14, 2019

@priyankatuteja Can you have a look?
P.S: This is very early draft, and might contain some typos.

@divyanshj16
Copy link
Contributor Author

divyanshj16 commented Nov 18, 2019

@rohitgeo, @cyber-aman, @Yongyao, @AtmaMani. Could anyone review this?

@AtmaMani
Copy link
Contributor

@divyanshj16 is this ready? The title says draft

@divyanshj16 divyanshj16 changed the title adds initial draft of pspnet guide PSPNet Guide Dec 11, 2019
@divyanshj16
Copy link
Contributor Author

divyanshj16 commented Dec 11, 2019

@AtmaMani
Yeah, this is ready. I have changed the title.

@AtmaMani
Copy link
Contributor

@Yongyao could you review this

@@ -0,0 +1,335 @@
{
Copy link
Contributor

@Yongyao Yongyao Dec 11, 2019

Choose a reason for hiding this comment

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

"and its paper has more than 1500 citations." -> this number can change quickly. Perhaps we can just say "its paper is one of the most cited papers in the field".


Reply via ReviewNB

Copy link
Contributor Author

@divyanshj16 divyanshj16 Dec 17, 2019

Choose a reason for hiding this comment

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

changed it to "its paper is highly cited by the computer vision community"

@@ -0,0 +1,335 @@
{
Copy link
Contributor

@Yongyao Yongyao Dec 11, 2019

Choose a reason for hiding this comment

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

We can rephrase the sentence to "For a detailed review of CNNs, please review Stanford University's CS231n: Convolutional Neural Networks for Visual Recognition course." We can provide a hyperlink if we think that link wont change in future


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -0,0 +1,335 @@
{
Copy link
Contributor

@Yongyao Yongyao Dec 11, 2019

Choose a reason for hiding this comment

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

  1. Can you spell out FCN? People might not know it at this point.
  2. Maybe we should soften the tone a bit. Is it always better? If no, we could change it to "Why PSPNet architecture could be an improvement over FCN based segmentation?


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Done
  2. Moderated the tone.

@@ -0,0 +1,335 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. The legend/label of this figure can be improved. Maybe put them on the top or at the bottom
  2. Consider numbering figures. For example, Figure 1. Image comparing FCN and PSPNet [1]. Same thing for the rest of guide

Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. How can I do that? The label is at the bottom and the legend is on the right. This image is directly from the paper. It seems to be a good for me.
  2. Since I am not using figure names in the text, is it still important to add the figure numbers?

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. If it's hard to change, that's fine.
  2. Yes, adding figure number is considered a good convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Left 1. Did 2.
Thanks.

@@ -0,0 +1,335 @@
{
Copy link
Contributor

@Yongyao Yongyao Dec 11, 2019

Choose a reason for hiding this comment

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

Heading "How to set scales in arcgis.learn" looks too small.


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Increased the heading.

@@ -0,0 +1,335 @@
{
Copy link
Contributor

@Yongyao Yongyao Dec 11, 2019

Choose a reason for hiding this comment

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

Heading is too small


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -0,0 +1,335 @@
{
Copy link
Contributor

@Yongyao Yongyao Dec 11, 2019

Choose a reason for hiding this comment

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

  1. Heading is too small
  2. To be consistent, can we change the title to "Implementation in arcgis.learn"

Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -0,0 +1,335 @@
{
Copy link
Contributor

@Yongyao Yongyao Dec 11, 2019

Choose a reason for hiding this comment

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

It would be better if you can follow the convention of citing a web page (different from a scientific paper).

For example, [author name, article title, url, accessed date] -> Howard Jeremy. Fastai - Dynamic U-Net. https://www.youtube.com/watch?v=0frKXR-2PBY. Accessed 2 September 2019.

You can refer to https://owl.purdue.edu/owl/research_and_citation/mla_style/mla_formatting_and_style_guide/mla_works_cited_electronic_sources.html for more detail.


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@Yongyao
Copy link
Contributor

Yongyao commented Dec 11, 2019

@divyanshj16 Great work. I added some comments mostly about format.

@divyanshj16
Copy link
Contributor Author

Thanks @Yongyao. I have incorporated your changes. Please see my comment on your legend\label comment.

Copy link
Contributor

@Yongyao Yongyao left a comment

Choose a reason for hiding this comment

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

@divyanshj16 Thanks. LGTM. @AtmaMani

@AtmaMani
Copy link
Contributor

AtmaMani commented Dec 19, 2019

@divyanshj16 I posted some suggestions can you review them? Further, can you remove the talk notebook and send it to the conference_talks branch as a separate PR?

@divyanshj16
Copy link
Contributor Author

divyanshj16 commented Dec 27, 2019

@AtmaMani I have incorporated your changes.
Are you talking about moving the talks folder, which needs to be moved to conference_talks branch as a seperate PR?

Copy link
Contributor

@divyanshj16, yes please remove the talks from this PR and send it to the conference_talks branch in a separate PR.

@divyanshj16
Copy link
Contributor Author

@AtmaMani
there is already a talks folder in conference_talks branch.

@AtmaMani AtmaMani merged commit 94cc264 into Esri:master Dec 31, 2019
@AtmaMani
Copy link
Contributor

@divyanshj16 thanks for the guide. @priyankatuteja @cyber-aman @Yongyao thanks for your detailed reviews.

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

Successfully merging this pull request may close these issues.

5 participants