121 lines
7.4 KiB
Markdown
121 lines
7.4 KiB
Markdown
# PR review
|
|
|
|
As GitHub does not support adding comments where things haven't changed,
|
|
the review will be done here. This is a hassle, but it is the only solution I
|
|
have as of now.
|
|
|
|
## Format
|
|
|
|
The square brackets are meant to be check boxes, and the numbers are line numbers in a specific file.
|
|
|
|
## ./latex/ising_model.tex
|
|
|
|
Nothing
|
|
|
|
## ./latex/sections/abstract.tex
|
|
|
|
[x] 12: variation => variance
|
|
[x] 14: variation => variance
|
|
[x] 16: "We estimate" => "We have estimated" as everything else is in past tense, and the rest of the sentence is in past tense as well.
|
|
|
|
## ./latex/sections/introduction.tex
|
|
|
|
[x] 10: "Magnetic materials can be classified" => "Magnetic materials are classified". I changed this, since they are actually classified
|
|
[x] 11: consist => consists. "One of the groups" is singular.
|
|
[ ] 12: Is the saturation optional, or will they become saturated when exposed to a magnetic field?
|
|
Valid point, they do become saturated so maybe rephrase
|
|
[x] 26: "as well as algorithms ..." => "as well as the algorithms ..."
|
|
[x] 28: "we conclude ..." => "we will conclude ..."
|
|
|
|
## ./latex/sections/methods.tex
|
|
|
|
[x] 6: consist => consists. "The Ising model" is singular.
|
|
[x] 6: though => thought
|
|
[x] 7: "the length of the lattice ..." => "the length of a lattice ...". We're not talking about a specific lattice.
|
|
[x] 26: remove "will", as we are assuming a 2x2 now and not later in the paper.
|
|
[x] 33: denote => denotes. $\langle k l \rangle$ is singular.
|
|
[ ] 34: Change to present tense, as we are considering it in this section and not later in the paper.
|
|
I think I've fixed it, however, tense is apparently not my jam these days XP
|
|
[x] 92: "Values of total energy ..." => "Values of the total energy ..."
|
|
[x] 98: "by number of spins ..." => "by the number of spins ..."
|
|
[x] 162: Why are the equations for heat capacity and susceptibility in different environments?
|
|
Well, that is an excellent question! That I do not have an answer to, but I changed it so that both are in equation environment.
|
|
[x] 208: transition => transitions. "the Ising model" is singular.
|
|
The singular thing is really something I should have a look into!
|
|
[x] 209: discontinous => discontinuous
|
|
[x] 211: with => to
|
|
[x] 217: "with an lattice" => "with a lattice"
|
|
[x] 223: Should perhaps create a reference to his paper?
|
|
I agree, I had already added a reference but had forgotten to cite it in the report. I did not include a page number, don't think it is necessary since its what the paper finding?
|
|
[x] 224: "using finite lattices ..." => "using the finite lattices'". We are referring to specific lattices and "lattices" should be possessive. I would rewrite the second part to be "using the critical temperature of finite lattices of different sizes and linear regression."
|
|
I agree, it does make more sense
|
|
[x] 230: depend => depends. "the next sample" is singular.
|
|
[x] 239: require => requires. "generating new random states" is singular.
|
|
[x] 240: detaild => detailed
|
|
[x] 241: criterias => criteria. the plural of criterion is criteria.
|
|
[x] 242: Figure => Algorithm, since we can remove the figure environment enclosing the algorithm environment.
|
|
[ ] More details are specified in the next comment below.
|
|
[ ] 244: A Monte Carlo cycle consists of attempts of flipping a spin, and not a single attempt to flip a spin.
|
|
Not sure how to rephrase the sentence to make it correct, any specific suggestions?
|
|
[x] 261: Instead of having the algorithm inside a figure, you can use the H specifier in the algorithm environment (which you already do). You can read more about it [here](https://tex.stackexchange.com/questions/231191/algorithm-in-revtex4-1). I have already tested that it works, and it makes it so we can actually refer to it as an algorithm and not a figure.
|
|
This might have fallen through the cracks for me since I just copy-pasted from metropolis.tex
|
|
[x] 284: reach => reaches. "The Markov process" is singular.
|
|
[ ] 283: I changed the paragraph to include a better introduction of the concept burn-in time, do you think the "flow" is better now?
|
|
[x] 295: implementation => execution. Implementation is the writing of the code, while the runtime is during execution of the program. I would also say to avoid potentially having the program run endlessly.
|
|
[ ] 292: Do you have a good way of including "I would also say to avoid potentially having the program run endlessly" in the sentence?
|
|
[ ] 297: Change of tense. We have used present tense for most of this section, so we should continue with that.
|
|
Added this into a new point, since the line numbers have shifted now
|
|
[ ] 294: So for the paragraph starting with "We used a pattern..." should be rewritten to present tense?
|
|
[ ] 300: Lack of if-tests does not hinder parallelization, but it can hinder compiler optimizations.
|
|
Suggested change "... avoids the use of if-tests, so we can take advantage of the compiler optimization."
|
|
[x] 318: used => use. Keep to the present tense for consistency.
|
|
[x] 320: used => use.
|
|
[x] 320: "the Python ..." => "the Python ...". Double space.
|
|
[x] Included the normal distribution method from SciPy as well
|
|
[x] 321: used => use.
|
|
[x] 321: profiler => profiling.
|
|
[x] 321: Remove Scalasca as we only use Score-p now. (Easier to deal with).
|
|
|
|
## ./latex/sections/results.tex
|
|
|
|
[x] 15: Should have a line break to indicate new paragraph.
|
|
[ ] 15: It might be relevant, but it's a lot more work. we have all the tools necessary to produce the plots, so it depends on how much work we can do.
|
|
I suggest we leave it out for now, I'll comment it out in case we want to use it
|
|
[x] 89: onordered => unordered
|
|
[ ] 97: More accurately, we generated 10M samples per temperature point.
|
|
True, so "...$10$ million samples of spin configurations for lattices..." => "...$10$ million samples of spin configurations, per temperature, for lattices..."
|
|
[ ] 77: Approve change "Histogram $T = 1.0 J / k_{B}$" => "Distribution of values of energy per spin, when temperature is $T = 1.0 J / k_{B}$"
|
|
[ ] 85: Approve change "Histogram $T = 2.4 J / k_{B}$" => "Distribution of values of energy per spin, when temperature is $T = 1.0 J / k_{B}$"
|
|
[x] 100: Should be that the program allocated, so that the reader understands that it does the division automatically.
|
|
I agree!
|
|
[x] 101: thread => threads
|
|
[ ] 105: We used a profiler. And it's not just for ensuring load balance among threads, it's also for each process, and we use it to see that data transfer is minimal.
|
|
So instead of "We ran a profiler" it should be "We used a profiler"? Could you suggest a way of formulating the sentence so that the info is correct?
|
|
[ ] 106: was => were
|
|
My previous comment might make this unnecessary?
|
|
[ ] 113: Included description of variables in caption
|
|
[x] 116: deacrese => decrease
|
|
[x] 117: "suggesting the system ..." => "suggesting that the system ...".
|
|
[x] 118: increase => increases. "The system energy" is singular.
|
|
[x] 127: observed => observe.
|
|
[x] 128: "capacity the" => "capacity when the"
|
|
[x] 135: show => shows.
|
|
[x] 136: "Since shape ..." => "Since the shape"
|
|
|
|
## ./latex/sections/conclusion.tex
|
|
|
|
[x] 14: Didn't we arrive at 5000?
|
|
Haha, ofc we did!
|
|
[x] 22: temperature => temperatures
|
|
[x] 24: variation => variance
|
|
[x] 26: variation => variance
|
|
|
|
## ./latex/sections/appendices.tex
|
|
|
|
[x] 95: "The squared energy" => "The squared energy and magnetization functions are then"
|
|
I agree, that sound a lot better! I just ended up wtiting something somewhat descriptive in lack of words.
|
|
[x] 130: he => the
|
|
Also, I changed the Eq. => to Equation
|
|
|
|
[x] 188: General, Included captions for figures in section
|