search

datacarpentry / python-ecology-lesson

Data Analysis and Visualization in Python for Ecologists
https://datacarpentry.org/python-ecology-lesson
Other
160 stars 310 forks source link

Hot007 issue307 update guide #460

Closed hot007 closed 4 years ago

hot007 commented 4 years ago

Add content to Instructors Guide to Python Ecology lesson, particularly the final (SQL) section. (part of instructor check-out).

maxim-belkin commented 4 years ago

Hi @hot007! Thank you for the PR!

I like your changes but I'm curious as to why you change some headings, e.g., "05-merging-data" to "05-merging-data-with-pandas". In this particular example, episode title is "Combining DataFrames with Pandas" and the filename is 05-merging-data.md (which, I believe, was used for titles). I think it makes sense to use either episode titles OR names of the files. I suspect using abbreviated episode titles might be confusing.

Other than that -- it looks great! thank you!

hot007 commented 4 years ago

Hi Maxim, Ah, I was trying to get the title closer to the episode title (explicitly saying "with pandas" but definitely fine to reject that bit! No worries! -Claire

maxim-belkin commented 4 years ago

Sounds good. Please update the PR

Hi Maxim, Ah, I was trying to get the title closer to the episode title (explicitly saying "with pandas" but definitely fine to reject that bit! No worries! -Claire

Excellent! Could you please update the PR accordingly then? I think these headings can be episode titles, file names (without .md), or "Episode X: Episode title". Using episode titles make the most sense to me but I don't mind if you keep them as they were.

Thank you for your work and let me know if you need help or advice.

Maxim

hot007 commented 4 years ago

Okay, I've made the requested changes I hope, please let me know if I missed anything. Thanks for the feedback, I'm learning about this {: .} syntax, that's really helpful, apologies I had it wrong, the preview for me isn't parsing those bits so I couldn't see it wasn't working. I haven't changed the other section headings though, do we really want to call one of the sections "07-visualization-ggplot-python" when ggplot has been replaced with plotnine? Or should we rename that episode file? Similarly for Eps 6, 8 and 9, whose filenames are now substantially out of line with the episode titles. Actually I will make another commit rolling back my changes to the headings, but just noting the filenames probably should be changed so that changes like this can be made in the instructor's guide.

maxim-belkin commented 4 years ago

I haven't changed the other section headings though, do we really want to call one of the sections "07-visualization-ggplot-python" when ggplot has been replaced with plotnine? Or should we rename that episode file? Similarly for Eps 6, 8 and 9, whose filenames are now substantially out of line with the episode titles. Actually I will make another commit rolling back my changes to the headings, but just noting the filenames probably should be changed so that changes like this can be made in the instructor's guide.

My personal preference is to use "Ep. #: Episode Title" headings but what you did here makes perfect sense. Of course, we should keep episode file names in sync with the content, e.g. ggplot -> plotnine, etc., but these discrepancies should be addressed in separate PRs (1 file change per PR).

I'm going to merge this PR as it seems to be ready. Thank you for the excellent contribution! If you'd like to work on any of the open issues -- let me know and I'll be happy to help!

🏅