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

ENH: roundabout_simplification() notebook example #392

Merged
merged 10 commits into from Sep 14, 2022

Conversation

gregmaya
Copy link
Contributor

What it says on the tin!

@gregmaya gregmaya changed the title ENH: roundabout_simnplification() notebook example ENH: roundabout_simplification() notebook example Sep 11, 2022
Copy link
Member

@jGaboardi jGaboardi left a comment

Choose a reason for hiding this comment

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

Nice work, @gregmaya. Here's some initial comments:

  • let's run black on the notebook for consistent string quotations, spacing, etc.
  • give all text a thorough scour for spelling, grammar, etc. (I can help with this later).
  • we should consider some static matplotlib plots in lieu of the .explore() method. Using .explore() is great for live demos, etc. but they do not render in notebook HTML.

@jGaboardi
Copy link
Member

cc @amorfinv

@martinfleis
Copy link
Member

martinfleis commented Sep 11, 2022

we should consider some static matplotlib plots in lieu of the .explore() method. Using .explore() is great for live demos, etc. but they do not render in notebook HTML.

They do render in HTML, that it not an issue (see https://geopandas.org/en/stable/docs/user_guide/interactive_mapping.html) but they tend to be much heavier to load as you need to ship all the data. So agree with @jGaboardi that we should minimise them.

edit: better see https://momepy--392.org.readthedocs.build/en/392/examples/roundabout_simplification.html

@martinfleis
Copy link
Member

Notebooks are pain to review....

  1. this should not be in Examples but in the User guide. I will create the section for that later today. That also affects the language of the notebook and what is and is not included.
  2. We need to explain the problem this tries to solve. See https://momepy--392.org.readthedocs.build/en/392/user_guide/elements/preprocessing.html as a reference.
  3. this bit should be removed. The reason I have a similar text in the other example is because I needed to give a proper reference to the original source. In this case, this is the original source.
- **Part of GSOC2022** 
-
- The function was originally developed by [Greg Maya](https://github.com/gregmaya) and supervised by [Martin Fleischmann](https://github.com/martinfleis), [James Gaboardi](https://github.com/jGaboardi) and [Andres Morfin](https://github.com/amorfinv)
-
-It depends on the following packages:
-
-```
- momepy
- geopandas
- osmnx
-```
  1. use osmnx (as a code block) rather that OSMNX to be consistent with the rest of the docs.
  2. crs then should be CRS (capital as it is an abbreviation and no need for bold).
  3. Note that two things help imporve the reults: 1. reproject the the network to a projected CRS (in meters)
    Does it just "improve" the result or is geographic just wrong to use?

  4. use full words in text and comments (e.g. geoms -> geometries)
  5. either use to_crs with a local_crs or ox.project_graph(G). Doing both is redundant. I'd prefer keeping the osmnx method as it is more universal. You can change the place and the code will work.
  6. use ax.set_axis_off() instead of plt.axis("off"); as we use elsewhere in the docs.

As mentioned above, give it a thorough proofreading.

@martinfleis
Copy link
Member

You can update this branch from main and move the notebook now.

git fetch upstream
git merge upstream/main

@jGaboardi
Copy link
Member

@gregmaya Once you have addressed @martinfleis's comments above, retag me for a review.

@gregmaya
Copy link
Contributor Author

Sure. I admit I might have to leave it for tomorrow but hopefully when you're up things will look much better.

@martinfleis
Copy link
Member

@gregmaya you will also need to list it in ToC here https://github.com/pysal/momepy/blob/main/docs/user_guide/preprocessing/preprocessing.rst to show in docs.

@gregmaya
Copy link
Contributor Author

@jGaboardi and @martinfleis feel free to review when you have some time.

I know GSOC has a hard deadline today so I'm going to focus on documentation and putting up some other draft PR with some helper function I showed you guys before. That way can properly link them in the overview and I'll continue to develop things this week to hopefully tight things up a bit more.

@martinfleis martinfleis marked this pull request as ready for review September 12, 2022 09:54
@amorfinv
Copy link
Contributor

@gregmaya Unsure how to add comments to a notebook here.

Small comments on first paragraph:

  • OpenStreetMap should not have spaces.
  • also there is a "sotred" that should be "sorted"

I think I will make a PR to your branch for any other comments as that should be easier

@amorfinv
Copy link
Contributor

Hey @gregmaya,

Fixed a couple of more typos here. I would not merge my PR into your branch haha..but if you want you can copy paste the markdown. I did not really make any changes in the code.

@gregmaya
Copy link
Contributor Author

Thanks, @amorfinv ! I copied over your corrections. I appreciate the extra pair of eyes.

@jGaboardi
Copy link
Member

@gregmaya A couple more things for the test:

  1. Add a period after the first sentence ending with "...using momepy.roundabout_simplification()."
  2. The working in the second sentence in the second text chunk is a bit it awkward. Change "The underneath code" to "The code below."

@martinfleis
Copy link
Member

I've left some suggestions here gregmaya#2 (review). You may even merge that directly in.

Copy link
Member

@jGaboardi jGaboardi left a comment

Choose a reason for hiding this comment

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

Conditional approval pending your incorporation of @martinfleis's comments.

Copy link
Member

@martinfleis martinfleis left a comment

Choose a reason for hiding this comment

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

Thanks!

@martinfleis martinfleis merged commit a7ba4a2 into pysal:main Sep 14, 2022
@gregmaya gregmaya deleted the rabs_notebook_example branch January 31, 2023 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants