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

Use this to comment on everything #14

Closed
wants to merge 119 commits into from
Closed

Conversation

mwcraig
Copy link
Member

@mwcraig mwcraig commented May 30, 2019

No description provided.

Copy link
Contributor

@laurenmarietta laurenmarietta left a comment

Choose a reason for hiding this comment

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

I am so sorry that this shows up as 73 individual comments, that number is truly overwhelming! I hope it makes it better if I say that probably 80% of those are repeated comments about either typos, the plot font size being very small, or heading formatting. For the typos, I tried to use git’s new suggestion feature so hopefully it will be very easy to merge those, at least!

Also, I won’t include individual comments about it as I am sure you are aware, but the links between notebooks are broken or blank. (Smart not to fill those in until the notebook ordering/naming is settled.)

Generally, I think this is a fantastically rigorous guide - even with 4 unwritten chapters! I really appreciate how you lay out and narrate the different reduction concepts and processes, and how you include well-commented and useful code - that I could even copy and use for myself if I needed.

My only real concern is regarding the structure - I think the notebook order could be rearranged in some places, and also there could be more descriptive titles for the sets of notebooks and the individual notebooks themselves. But that might make more sense to address when they are all complete and the guide can be considered as a whole.

mwcraig and others added 9 commits June 3, 2019 09:47
…image.ipynb

Co-Authored-By: Lauren Chambers <Lauren.m.chambers96@gmail.com>
…image.ipynb

Co-Authored-By: Lauren Chambers <Lauren.m.chambers96@gmail.com>
Co-Authored-By: Lauren Chambers <Lauren.m.chambers96@gmail.com>
Co-Authored-By: Lauren Chambers <Lauren.m.chambers96@gmail.com>
@mwcraig mwcraig mentioned this pull request Jun 5, 2019
@mwcraig
Copy link
Member Author

mwcraig commented Jun 7, 2019

Thanks for all of the detailed feedback, @laurenmarietta! I think I've addressed everything you raised either by fixing it or making an issue. The comments were very useful!

@mwcraig mwcraig closed this Jun 7, 2019
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

3 participants