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

First Round Edits from Comments #4

Merged
merged 51 commits into from
Jan 4, 2018
Merged

First Round Edits from Comments #4

merged 51 commits into from
Jan 4, 2018

Conversation

jbae11
Copy link
Contributor

@jbae11 jbae11 commented Oct 11, 2017

First Round Edits from Comments.
#3

@jbae11 jbae11 requested a review from gwenchee October 18, 2017 00:02
@jbae11
Copy link
Contributor Author

jbae11 commented Oct 18, 2017

@gwenchee let me know what you think. If you like it, we will ask prof.Huff for review!

@gwenchee
Copy link
Contributor

I made some more edits to the current draft and pushed to the jbae11 master branch. Is there a reason why in section 2.3 Analytical solution, you didn't include the fuel fab facilities? If no, i can add them. @jbae11

Copy link
Member

@katyhuff katyhuff left a comment

Choose a reason for hiding this comment

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

I was hoping for a much more complete document than this. Many grammatical errors remain, the discussion is sparse, there are many placeholders ([insert uncertainty]), and the document relies far too heavily on code examples. Please go back through this completely and try to arrive at a complete, camera ready document. For specific comments, you are welcome to drop by my office. Generally, my comment is this: if you remove the code examples from the document, the document makes no sense at all. Rewrite this document in such a way that the code examples are illustrative, referenced by the text, and not essential to the understanding of the tests.

Other comments:

  • eradicate the use of the word "would"
  • The tests are not appropriate for an itemized list (a list shouldn't span multiple pages... it should be a part of a sentence). Consider subsections or paragraphs for each. OR, if you must use a list, list the main purpose of each test in a single line, then discuss the details of each test elsewhere (e.g. in subsections)
  • The grammar is in need of a re-read (Weird phrases like "The method to how" will become obvious if the document is read aloud by the authors.... )

README.md Outdated
@@ -1,7 +1,7 @@
# ddca_numerical_exp
Numerical Experiment for CYCLUS Demand-Driven Deployment

The repository is part of an effort to add demand-driven deployment
hi The repository is part of an effort to add demand-driven deployment
Copy link
Member

Choose a reason for hiding this comment

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

?

README.md Outdated
@@ -1,7 +1,7 @@
# ddca_numerical_exp
Numerical Experiment for CYCLUS Demand-Driven Deployment

The repository is part of an effort to add demand-driven deployment
hi The repository is part of an effort to add demand-driven deployment
capabilities into the [CYCLUS](github.com/cyclus/cyclus) framework.
PI: Kathryn Huff
Copy link
Member

Choose a reason for hiding this comment

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

Again, I'm not the PI. Anthony is the PI. I'm the co-PI. Please just list me as last author instead.

@gwenchee
Copy link
Contributor

@katyhuff The report is ready for your review.

@katyhuff
Copy link
Member

Thanks! I'll look it over asap.

@katyhuff katyhuff merged commit eda709a into arfc:master Jan 4, 2018
@katyhuff
Copy link
Member

katyhuff commented Jan 4, 2018

bitmoji

gwenchee pushed a commit that referenced this pull request Sep 29, 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.

3 participants