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

convert static plotting from base r to ggplot2 for episode 12 and 13 #190

Merged
merged 5 commits into from
Jul 12, 2018
Merged

Conversation

annefou
Copy link

@annefou annefou commented Jul 12, 2018

updated episode 12 and 13: ggplot2 is now used instead of base r and rasterVis as requested in #133

plot(NDVI_HARV_stack,
zlim = c(.15, 1),
nc = 4)
# convert to a df for plotting in two steps,
Copy link
Contributor

Choose a reason for hiding this comment

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

These lines look identical to lines 217-225. Can they be removed or is there a difference I'm not seeing?

Copy link
Author

Choose a reason for hiding this comment

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

Yes it is a bit heavy but because we changed NDVI_HARV_stack (divided by 10000), we need to recreate NDVI_HARV_stack _df again, isn't it?

@ErinBecker
Copy link
Contributor

Thanks @annefou, this looks really good. I left some notes - one place where there appears to be about 10 lines of duplicated code, and then a few places where I think we can shorten the ggplot calls. I'm happy to merge this as is (once the check completes) and make further fine-tuning adjustments later, so don't worry about following up on my review. I can test out the adjustments and see if they work. Thanks again!

@ErinBecker ErinBecker closed this Jul 12, 2018
@ErinBecker ErinBecker reopened this Jul 12, 2018
Copy link
Contributor

@ErinBecker ErinBecker left a comment

Choose a reason for hiding this comment

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

I see, thanks for clarifying.

# one single column called 'variable'
melt(id.vars = c('x','y','optional'))

# view a plot of all of the rasters
Copy link
Contributor

Choose a reason for hiding this comment

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

This also looks identical to lines 227-232

> `par(mfrow=c(2, 2))` to create a 2x2 tiled layout. When you are done, be sure to
> reset your layout using: `par(mfrow=c(1, 1))`.
>
> `grid.arrange` from gridExtra R package. Make sure you load it by using:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not clear on why we're introducing a new library in an exercise. I think that will be really, really challenging for people. Is this something that you can't do with facet_grid?

Once the names for each band have been reassigned, we can render our plot with
the new labels using `names.attr=rasterNames`.
the new labels using a`labeller`.
Copy link
Contributor

Choose a reason for hiding this comment

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

missing space?

ggtitle("Landsat NDVI - Julian Days \nHarvard Forest 2011") +
theme(plot.title = element_text(hjust = 0.5),
# remove x and y-axis title, text and ticks
axis.title.x=element_blank(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can do this by using theme_void() on line 207 (before the theme call to center the title). Then we can get rid of lines 209-214 (I think).

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Yes if we use:
theme_void() + theme(plot.title = element_text(hjust = 0.5),
we can remove the rest.

@ErinBecker
Copy link
Contributor

@annefou - the check passed! I'm going to merge this now and we can make some minor adjustments as needed in a later PR.

@ErinBecker ErinBecker merged commit c899edd into datacarpentry:master Jul 12, 2018
zkamvar pushed a commit that referenced this pull request Feb 7, 2023
convert static plotting from base r to ggplot2 for episode 12 and 13
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.

None yet

2 participants