aloknsingh / ibm_r4ml_biganalytics

Apache License 2.0
0 stars 11 forks source link

Editing the Notebook: R4ML_Data_Preprocessing_and_Dimension_Reduction.ipynb #3

Closed MadisonJMyers closed 6 years ago

MadisonJMyers commented 6 years ago

Alok, it is very clear that you are skilled and knowledgeable about what you are writing about! I want to help you better communicate that to the reader. I can see a lot of thought went into the layout and organization and I must compliment you on that! There are some grammatical and formatting errors that I suggest you implement, however. Please see my comments below:

aloknsingh commented 6 years ago

fix as per review

xwu0226 commented 6 years ago

@aloknsingh It would be ideal if @MadisonJMyers has a chance to do another check over your changes for accuracy and close the issue herself. Thanks!

MadisonJMyers commented 6 years ago

After review, I've found a few minor edits:

Other than that the notebook has improved a lot and flows well! Great work.

aloknsingh commented 6 years ago

Hi Madison

in ref to "Under 2.2 I don't see the diagram you had shown me about time of the data scientist. Will you be adding onto this section?"

it's there I am not sure why you can't see it

aloknsingh commented 6 years ago

fixed as per comments

MadisonJMyers commented 6 years ago

Hm I'm not sure why the diagram isn't showing? Can others see it? If so perhaps it is my browser or something else. Other than that I only noticed one edit:

Once that's complete I will close the issue! Great work!

xwu0226 commented 6 years ago
screen shot 2017-10-26 at 4 18 10 pm

I don't see it either. Is this the place?

MadisonJMyers commented 6 years ago

Yes that's where it should be. Alok showed me in person, but I'm not sure why it's not showing up on our computers. I'm using Firefox- what browser are you using?

xwu0226 commented 6 years ago

neither Chrome nor Safari shows

MadisonJMyers commented 6 years ago

Hm, have we encountered this problem before? I'm not sure how to fix it. @aloknsingh any ideas?

xwu0226 commented 6 years ago

maybe Alok showed it in DSX ? and the diagram can be only rendered well in DSX jupyter? directly showing the notebook file in browser may have some issue? @aloknsingh

aloknsingh commented 6 years ago

may be you want to double click?

aloknsingh commented 6 years ago

@MadisonJMyers can you pl have all the comments at one shot rather than incremental thanks

aloknsingh commented 6 years ago

this is direct link https://dataplatform.ibm.com/analytics/notebooks/fa911700-b14a-47d8-a8e9-4b8f82a0d0da/view?projectid=9f9a9e62-8ff0-42ef-a285-4bdff61e18cd&context=analytics

aloknsingh commented 6 years ago

done

MadisonJMyers commented 6 years ago

Yeah, I'm giving you all of my comments together at one time. My comments that follow my original comment, are in response to your comment "fixed as per comments". When I see that you have claimed implementation, I then review to make sure nothing was missed. When I reviewed, I noticed a few errors, which I posted.

Regarding the diagram: you cannot "double click" it. There's no button. When trying to access your DSX notebook, the access is forbidden.

2.3 and 2.4 are still missing periods.

aloknsingh commented 6 years ago

Ok.

on the review side. there are many instance where we use period in end and not period in end. Why this differences (wrt to initial comments)? I think we should either have all period or no period in the header and sub-header. Also if we see general paper or book usually there are no period in heading. But they do include ?

MadisonJMyers commented 6 years ago

Great question!

So all sentences that are not in the header (which is what I was referring to in 2.3 and 2.4) need to end in punctuation. Usually it's a period but in instances when you are going to follow up with a list, you need a colon.

I agree that consistency is important! I usually ask for a period to end bullet points, but not headers. Others may ask you to do something differently. I think, like you pointed out, consistency is most important.

All that's left on your end for this is to end your sentences in 2.3 and 2.4 with periods. (There are two sentences without them.) Once that is complete, I'll close the issue.

If any of this is ever confusing, don't hesitate to reach out to me and ask questions. I'm happy to help!

aloknsingh commented 6 years ago

I was referring to other sections like in the

1) Introduction and Exploratory Data Analysis -) there is no period in header "Introduction" and it's sub section -) similar there is no period is other places

2) Data Preparation and Dimen -) There is no period in Introduction

3) Classification Model using SVM -) There is no period in any of the 1 2 3 sections

I think in general if you open any textbook they don't put period in the header or sub-header and it is considered bad practice to put. so in that spirit I suggest lets put no period in the header , subheader

aloknsingh commented 6 years ago

@xwu0226 and @MadisonJMyers seems like browser can't render all the element of the ipynb. It works fine in DSX . here is a preview .download these files

https://github.com/SparkTC/r4ml/pull/55/files

MadisonJMyers commented 6 years ago

@aloknsingh if you disagree, feel free to get someone else to review in addition to me. To be clear, I am not asking you to put periods in your headers. I am simply asking you to close all of your sentences with punctuation. I will close the issue once this is complete.

aloknsingh commented 6 years ago

Ok, I see your point. i think I will modify 2.3 2.4 and then change all the header to be non period. I think this should be consistent. How does it sounds

MadisonJMyers commented 6 years ago

Great! Let me know once you've done that.

MadisonJMyers commented 6 years ago

I see that you've implemented the two periods. Awesome. I'll go ahead and close this issue, but I'd recommend having it reviewed once again as some phrasings are still awkward. I also recommend figuring out why your diagram in 2.2 isn't showing up for people. If this issue persists, you may want to remove that section. Other than that, good work.