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

Improving Episode 5 #47

Closed
vinisalazar opened this issue Apr 15, 2021 · 5 comments
Closed

Improving Episode 5 #47

vinisalazar opened this issue Apr 15, 2021 · 5 comments

Comments

@vinisalazar
Copy link
Contributor

Hi, I have a couple of concerns for Episode 5:

  • The first is in the surface_HARV = rioxarray.open_rasterio("data/NEON-DS-Airborne-Remote-Sensing/HARV/DSM/HARV_dsmCrop.tif") line. In the setup page, instructions state that the downloaded data should be moved inside the geospatial-python directory and then unzipped. This may cause FileNotFoundErrors, that are easy to solve, but could be avoided. I propose that either the "data/" string is removed from the open_rasterio function OR that the setup instructions state that a data directory needs to be created (I believe the former is easier).
  • This is related to Improve aesthetics of geospatial plots, particularly axis ticks #22: There is a "Plotting Tip" callout in Episode 05, but plotting with Matplotlib is only introduced in Episode 06. I believe that this Plotting Tip could be somewhere else in the lesson, and there could a more detailed section on how to customize plots with Matplotlib. Since Is episode 6 necessary? #19 also discusses merging Episode 06, I don't know what would be the best way to tackle this.

I hope this is helpful for the discussion.

Best wishes,
V

@rbavery
Copy link
Collaborator

rbavery commented Apr 16, 2021

Definitely helpful, thanks @vinisalazar !

On the first point, when I unzip the folder on my Mac, the result is a data folder that contains the sub directories for each dataset. Is this not the case on your end? I can test this out on windows tomorrow, I thought that when the .zip is extracted, the result is always a folder called "data" that contains the subdirectories, so a data folder wouldn't need to be created.

On the second point, I agree we could improve the introduction of plotting with matplotlib + xarray's 'plot' methods, it'd probably be good to discuss briefly how they complement each other.

First, I think it'd be best to remove Episode 6's use of the reproject function following #19. The first part of Episode 7 could become episode 6, focusing on using reproject_match to align different rasters. We could also keep the challenge on masking no data values prior to plotting since this doesn't use matplotlib.

The second part of Episode 7 currently focuses on running a calculation to get a Canopy Height Model and making a pretty plot. This could formally introduce customizing plots with matplotlib and earthpy, since that sets up the later episodes.

I think it's ok to still keep the code that use plt.title in the episodes before this one, just to get folks gradually familiar with it before it is formally introduced, also because this lesson assumes that people have taken a novice python lesson that teaches matplotlib and exposes folks to methods and attributes. But I'm also not opposed to removing these until after the section on customizing plots.

@vinisalazar
Copy link
Contributor Author

vinisalazar commented Apr 16, 2021

Hi @rbavery,

Is this not the case on your end?

I tried it again and that is indeed the case. Sorry for that.

this lesson assumes that people have taken a novice python lesson that teaches matplotlib and exposes folks to methods and attributes.
On the second point, I agree we could improve the introduction of plotting with matplotlib + xarray's 'plot' methods, it'd probably be good to discuss briefly how they complement each other.

That is a fair point that people should be familiar with Matplotlib. Thinking about it now, expanding the "Plotting Tip" block in Episode 05 could be a way of having the Matplotlib introduction. And then on Episode 7, learners will already be more familiar. What do you think?

Edit: I actually reconsidered. Having the Plotting Tip in the middle of Episode 5 might be confusing. All plotting functions in that episode can be done with xarray's methods. In Episode 7, plotting functions depend more on Matplotlib. So maybe it would be nice to place it right before the "Harvard Forest Digital Terrain Model" plot, considering that Episode 6 will be merged.

@vinisalazar
Copy link
Contributor Author

I will submit a PR with a tentative approach.

@rbavery
Copy link
Collaborator

rbavery commented Apr 16, 2021

Great, that sounds good to me. If you don't get to it, I'll work on the episode 6 refactor to focus on reproject_match and dealing with no data values with masking. Thanks a bunch for your comments and help, much appreciated!

vinisalazar added a commit to vinisalazar/geospatial-python that referenced this issue Apr 16, 2021
…arpentries-incubator#22).

  - Move the 'Plotting Tip' block from Episode 05 to Episode 06
  - Add a new block 'Customizing plots with Matplotlib in Episode 06
@rbavery rbavery closed this as completed Apr 21, 2021
@rbavery
Copy link
Collaborator

rbavery commented Apr 21, 2021

it's addressed see #19

rbavery pushed a commit that referenced this issue Oct 10, 2022
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

No branches or pull requests

2 participants