Added ipynb tutorial - membrane-protein.

[ojeda-e]

This PR fixes #68

Changes include:

• [x] Tutorial membrane-protein systems with posres.
• [x] Tutorial page updated.

[review-notebook-app[bot]]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[codecov[bot]]

Codecov Report

Merging #69 (2ee6898) into main (4bb851e) will not change coverage. The diff coverage is n/a.

[ojeda-e]

Here is the second tutorial ready for review @orbeckst, @lilyminium. Apologies for the delay.

I have a question regarding the reference edit: of the NhaA Dataset. I'm printing the reference with print(NhaA.DESCR). I am not sure that's enough, though. Would you please suggest to me the best way to add the reference here? I considered two options. a) Add a last section in the notebook with the reference. b) Add it at the beginning of the document where I give the short description of the simulation.

Thanks!

[orbeckst]

I probably won't get to it before the end of the week. Please ping me again if you haven't heard from me by Friday. Thanks.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T17:54:08Z ----------------------------------------------------------------

I haven't seen more_itertools before -- looks very interesting! However, I think you may be able to save this dependency by using numpy:

membrane_indexes[j] = np.split(lfs[n], np.where(np.ediff1d(lfs[n].residues.resids) > 1)[0] + 1)

It would also be easier to understand your code if you used more descriptive variable names. i and j, for example, are typically reserved for index integers in code. Here, though, they stand for an array and the leaflet name ("Upper"/"Lower") -- and you don't actually use i. Personally I'd not use n but instead rewrite the loop as for leaflet_indices, leaflet_name in zip(lfs, leaflets).

ojeda-e commented on 2021-09-02T06:21:53Z ----------------------------------------------------------------

Thanks for this. I changed the name of the variables and used an approach following your suggestion. I didn't know about np.ediff1d, thanks!

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T17:56:21Z ----------------------------------------------------------------

"In our the NhaA system, we can take use the phospholipid headgroups from the POPE and POPG molecules as an"

Some double words there -- In our NhaA system, we can use ?

lilyminium commented on 2021-08-31T18:06:35Z ----------------------------------------------------------------

"an all atom MD simulations"

simulations -> simulation

"Then, we can select the phosphate atoms in our Universe with"

phosphate -> "phosphate phosphorus"

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T17:58:10Z ----------------------------------------------------------------

"we can use leaflet MDAnalysis module to"

We can use the leaflet MDAnalysis module to ?

"To use the MDAnalysis LeafletsFinder analysis module we do:"

LeafletsFinder -> LeafletFinder

I would also just say ""To use the MDAnalysis LeafletFinder we do:" -- it's a class in the leaflet module, rather than an analysis module on its own.

lilyminium commented 3 years ago

"an all atom MD simulations"

simulations -> simulation

"Then, we can select the phosphate atoms in our Universe with"

phosphate -> "phosphate phosphorus"

---

View entire conversation on ReviewNB

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:07:41Z ----------------------------------------------------------------

"The variable P_headgroups, however, include all the lipids in our system"

Instead of "include all the lipids", it might be better to be more explicit -- "include the phosphorus atoms of all the lipids in our system". Otherwise it sounds like it might be selecting other atoms in the lipid as well.

ojeda-e commented on 2021-09-02T06:26:24Z ----------------------------------------------------------------

This is a good point, thanks.

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:09:59Z ----------------------------------------------------------------

Wording suggestions:

"To identifying consecutive residue numbers in a list we can user consecutive_groups from the more_itertools package. We can save the residues in each leaflet in the membrane_indexes dictionary with:"

identifying -> identify

user -> use

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:17Z ----------------------------------------------------------------

"After identifying the residues in each leaflet, we can define lower and upper leaflet in our system with:"

Suggestion: After identifying the residues in each leaflet, we can define the lower and upper leaflet in our system with:

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:18Z ----------------------------------------------------------------

This is a nice thorough explanation, especially of the number of bins and coordinate wrapping!

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:19Z ----------------------------------------------------------------

Membranecurvature -> MembraneCurvature

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:20Z ----------------------------------------------------------------

MembraneCuvature -> MembraneCurvature

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:21Z ----------------------------------------------------------------

assymetric -> asymmetric

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:22Z ----------------------------------------------------------------

For this and every other image, it'd be really nice if the color map / color bar had the same range, so that yellow / blue meant the same thing in both plots. The trickiest one to do this for would be the surface. For the curvatures this is pretty simple, but for the surfaces I guess this could be solved by just setting the range to (25, 36) and (62, 73).

ojeda-e commented on 2021-09-02T08:28:05Z ----------------------------------------------------------------

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:24Z ----------------------------------------------------------------

Hmmm... this sounds odd to me. I think convention is to use the upward-pointing-normal, right, in which case positive H should be convex?

I wouldn't say null, I would explicitly say H=0 . Otherwise you risk conflating 0 with np.nan or None.

If you keep the colorbars in the same range, you can compare the curvatures by looking at the colours instead of the max/min values, too.

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:45:06Z ----------------------------------------------------------------

I think "inflexions" is still a bit vague -- if I remember high school correctly, a saddle point is also a point of inflection. With Gaussian curvature the principal curvatures are both the same sign so "extrema" or minimum/maximum might be better phrasing.

ojeda-e commented on 2021-09-02T08:25:34Z ----------------------------------------------------------------

I changed it to "concave regions".

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:46:05Z ----------------------------------------------------------------

inteprolation -> interpolation

lilyminium commented 3 years ago

A nice and thorough tutorial, @ojeda-e. I'll let @orbeckst answer the question about references, since I think it's his data. I mostly found some typos but my concrete feedback on the notebook was:

• I think you can swap more-itertools for numpy
• I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

ojeda-e commented 3 years ago

Thanks, @lilyminium for the comments. I will work on them and will replace itertools with numpy.

I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised Sign of mean curvature #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

You're right and I appreciate you double-check this. I actually made a (terrible) mistake due to a previous mental association I had with the bwr colormap. This is totally my bad. The sign is the opposite, soH>0 is valleys. The sign in the calculation of mean curvature is negative, same as in the mean curvature function in curvature.py. I'm very sorry I made this mistake and I will fix it in the next push (for this tutorial). Thanks again :)

ojeda-e commented 3 years ago

Thanks for this. I changed the name of the variables and used an approach following your suggestion. I didn't know about np.ediff1d, thanks!

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

This is a good point, thanks.

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed it to "concave regions".

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

---

View entire conversation on ReviewNB

orbeckst commented 3 years ago

I hope my comments on https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion actually show up...

orbeckst commented 3 years ago

Er, I am not sure how, but I commented on the notebook https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion but that's not linked to the PR?

Now I know how: I clicked on the "standalone notebook" link in NBreview as that was the most obvious thing to click on. Sorry, hope the comments are still useful.

ojeda-e commented 3 years ago

Thanks for the review @orbeckst. I can read your comments in the notebook although never got any notification, thanks for the link! I made a mistake by assuming the protein was fixed. Now in retrospect, I have no clue why did I even assume so. I will work on your comments. Thanks again!

ojeda-e commented 2 years ago

Apologies for the delay @orbeckst. The last push includes the suggested changes. I hope the changes are aligned with the ones you had in mind. I tried to follow all of them, please let me know if I missed anything there.

The more relevant changes are:

• Added discussion on protein diffusion (At the beginning and some other places).
• Added appendix to determine protein diffusion via MSD (Appendix 1).
• Added appendix with residue selection by leaflet. (Appendix 2).
• Updated function to generate NGL widget.

Thanks.

ojeda-e commented 2 years ago

Thanks @hmacdope I pushed the suggested changes in the tutorials.rst. However, I couldn't see your comments in ReviewNB. Would you please send me a link? This ReviewNB is very buggy 💀

hmacdope commented 2 years ago

@ojeda-e I have left comments on the notebook on the standalone review thingo at https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion

The tutorial looks and reads amazing! Mostly just little nitpicks and clarifications.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

diffuses

because the simulation was carried out without position restraints

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

NhaA-membrane simulation box (~120 Å x 120 Å divided into 6 x 6 = 36 bins). This warning can happen when the box size fluctuates as in an NPT simulation.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:55Z ----------------------------------------------------------------

diffuses through the membrane

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:56Z ----------------------------------------------------------------

diffuses through the membrane

OR

diffuses in the plane of the membrane

orbeckst commented 2 years ago

Looks great @ojeda-e ! I added minor comments on the notebook (which posted to the PR this time because I learned to comment on the diff).

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T17:56:21Z ----------------------------------------------------------------

"In our the NhaA system, we can take use the phospholipid headgroups from the POPE and POPG molecules as an"

Some double words there -- In our NhaA system, we can use ?

lilyminium commented on 2021-08-31T18:06:35Z ----------------------------------------------------------------

"an all atom MD simulations"

simulations -> simulation

"Then, we can select the phosphate atoms in our Universe with"

phosphate -> "phosphate phosphorus"

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T17:58:10Z ----------------------------------------------------------------

"we can use leaflet MDAnalysis module to"

We can use the leaflet MDAnalysis module to ?

"To use the MDAnalysis LeafletsFinder analysis module we do:"

LeafletsFinder -> LeafletFinder

I would also just say ""To use the MDAnalysis LeafletFinder we do:" -- it's a class in the leaflet module, rather than an analysis module on its own.

lilyminium commented 3 years ago

"an all atom MD simulations"

simulations -> simulation

"Then, we can select the phosphate atoms in our Universe with"

phosphate -> "phosphate phosphorus"

---

View entire conversation on ReviewNB

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:07:41Z ----------------------------------------------------------------

"The variable P_headgroups, however, include all the lipids in our system"

Instead of "include all the lipids", it might be better to be more explicit -- "include the phosphorus atoms of all the lipids in our system". Otherwise it sounds like it might be selecting other atoms in the lipid as well.

ojeda-e commented on 2021-09-02T06:26:24Z ----------------------------------------------------------------

This is a good point, thanks.

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:09:59Z ----------------------------------------------------------------

Wording suggestions:

"To identifying consecutive residue numbers in a list we can user consecutive_groups from the more_itertools package. We can save the residues in each leaflet in the membrane_indexes dictionary with:"

identifying -> identify

user -> use

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:17Z ----------------------------------------------------------------

"After identifying the residues in each leaflet, we can define lower and upper leaflet in our system with:"

Suggestion: After identifying the residues in each leaflet, we can define the lower and upper leaflet in our system with:

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:18Z ----------------------------------------------------------------

This is a nice thorough explanation, especially of the number of bins and coordinate wrapping!

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:19Z ----------------------------------------------------------------

Membranecurvature -> MembraneCurvature

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:20Z ----------------------------------------------------------------

MembraneCuvature -> MembraneCurvature

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:21Z ----------------------------------------------------------------

assymetric -> asymmetric

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:22Z ----------------------------------------------------------------

For this and every other image, it'd be really nice if the color map / color bar had the same range, so that yellow / blue meant the same thing in both plots. The trickiest one to do this for would be the surface. For the curvatures this is pretty simple, but for the surfaces I guess this could be solved by just setting the range to (25, 36) and (62, 73).

ojeda-e commented on 2021-09-02T08:28:05Z ----------------------------------------------------------------

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:24Z ----------------------------------------------------------------

Hmmm... this sounds odd to me. I think convention is to use the upward-pointing-normal, right, in which case positive H should be convex?

I wouldn't say null, I would explicitly say H=0 . Otherwise you risk conflating 0 with np.nan or None.

If you keep the colorbars in the same range, you can compare the curvatures by looking at the colours instead of the max/min values, too.

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:45:06Z ----------------------------------------------------------------

I think "inflexions" is still a bit vague -- if I remember high school correctly, a saddle point is also a point of inflection. With Gaussian curvature the principal curvatures are both the same sign so "extrema" or minimum/maximum might be better phrasing.

ojeda-e commented on 2021-09-02T08:25:34Z ----------------------------------------------------------------

I changed it to "concave regions".

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:46:05Z ----------------------------------------------------------------

inteprolation -> interpolation

lilyminium commented 3 years ago

A nice and thorough tutorial, @ojeda-e. I'll let @orbeckst answer the question about references, since I think it's his data. I mostly found some typos but my concrete feedback on the notebook was:

• I think you can swap more-itertools for numpy
• I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

ojeda-e commented 3 years ago

Thanks, @lilyminium for the comments. I will work on them and will replace itertools with numpy.

I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised Sign of mean curvature #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

You're right and I appreciate you double-check this. I actually made a (terrible) mistake due to a previous mental association I had with the bwr colormap. This is totally my bad. The sign is the opposite, soH>0 is valleys. The sign in the calculation of mean curvature is negative, same as in the mean curvature function in curvature.py. I'm very sorry I made this mistake and I will fix it in the next push (for this tutorial). Thanks again :)

ojeda-e commented 3 years ago

Thanks for this. I changed the name of the variables and used an approach following your suggestion. I didn't know about np.ediff1d, thanks!

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

This is a good point, thanks.

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed it to "concave regions".

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

---

View entire conversation on ReviewNB

orbeckst commented 3 years ago

I hope my comments on https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion actually show up...

orbeckst commented 3 years ago

Er, I am not sure how, but I commented on the notebook https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion but that's not linked to the PR?

Now I know how: I clicked on the "standalone notebook" link in NBreview as that was the most obvious thing to click on. Sorry, hope the comments are still useful.

ojeda-e commented 3 years ago

Thanks for the review @orbeckst. I can read your comments in the notebook although never got any notification, thanks for the link! I made a mistake by assuming the protein was fixed. Now in retrospect, I have no clue why did I even assume so. I will work on your comments. Thanks again!

ojeda-e commented 2 years ago

Apologies for the delay @orbeckst. The last push includes the suggested changes. I hope the changes are aligned with the ones you had in mind. I tried to follow all of them, please let me know if I missed anything there.

The more relevant changes are:

• Added discussion on protein diffusion (At the beginning and some other places).
• Added appendix to determine protein diffusion via MSD (Appendix 1).
• Added appendix with residue selection by leaflet. (Appendix 2).
• Updated function to generate NGL widget.

Thanks.

ojeda-e commented 2 years ago

Thanks @hmacdope I pushed the suggested changes in the tutorials.rst. However, I couldn't see your comments in ReviewNB. Would you please send me a link? This ReviewNB is very buggy 💀

hmacdope commented 2 years ago

@ojeda-e I have left comments on the notebook on the standalone review thingo at https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion

The tutorial looks and reads amazing! Mostly just little nitpicks and clarifications.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

diffuses

because the simulation was carried out without position restraints

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

NhaA-membrane simulation box (~120 Å x 120 Å divided into 6 x 6 = 36 bins). This warning can happen when the box size fluctuates as in an NPT simulation.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:55Z ----------------------------------------------------------------

diffuses through the membrane

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:56Z ----------------------------------------------------------------

diffuses through the membrane

OR

diffuses in the plane of the membrane

orbeckst commented 2 years ago

Looks great @ojeda-e ! I added minor comments on the notebook (which posted to the PR this time because I learned to comment on the diff).

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T17:58:10Z ----------------------------------------------------------------

"we can use leaflet MDAnalysis module to"

We can use the leaflet MDAnalysis module to ?

"To use the MDAnalysis LeafletsFinder analysis module we do:"

LeafletsFinder -> LeafletFinder

I would also just say ""To use the MDAnalysis LeafletFinder we do:" -- it's a class in the leaflet module, rather than an analysis module on its own.

lilyminium commented 3 years ago

"an all atom MD simulations"

simulations -> simulation

"Then, we can select the phosphate atoms in our Universe with"

phosphate -> "phosphate phosphorus"

---

View entire conversation on ReviewNB

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:07:41Z ----------------------------------------------------------------

"The variable P_headgroups, however, include all the lipids in our system"

Instead of "include all the lipids", it might be better to be more explicit -- "include the phosphorus atoms of all the lipids in our system". Otherwise it sounds like it might be selecting other atoms in the lipid as well.

ojeda-e commented on 2021-09-02T06:26:24Z ----------------------------------------------------------------

This is a good point, thanks.

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:09:59Z ----------------------------------------------------------------

Wording suggestions:

"To identifying consecutive residue numbers in a list we can user consecutive_groups from the more_itertools package. We can save the residues in each leaflet in the membrane_indexes dictionary with:"

identifying -> identify

user -> use

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:17Z ----------------------------------------------------------------

"After identifying the residues in each leaflet, we can define lower and upper leaflet in our system with:"

Suggestion: After identifying the residues in each leaflet, we can define the lower and upper leaflet in our system with:

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:18Z ----------------------------------------------------------------

This is a nice thorough explanation, especially of the number of bins and coordinate wrapping!

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:19Z ----------------------------------------------------------------

Membranecurvature -> MembraneCurvature

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:20Z ----------------------------------------------------------------

MembraneCuvature -> MembraneCurvature

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:21Z ----------------------------------------------------------------

assymetric -> asymmetric

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:22Z ----------------------------------------------------------------

For this and every other image, it'd be really nice if the color map / color bar had the same range, so that yellow / blue meant the same thing in both plots. The trickiest one to do this for would be the surface. For the curvatures this is pretty simple, but for the surfaces I guess this could be solved by just setting the range to (25, 36) and (62, 73).

ojeda-e commented on 2021-09-02T08:28:05Z ----------------------------------------------------------------

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:24Z ----------------------------------------------------------------

Hmmm... this sounds odd to me. I think convention is to use the upward-pointing-normal, right, in which case positive H should be convex?

I wouldn't say null, I would explicitly say H=0 . Otherwise you risk conflating 0 with np.nan or None.

If you keep the colorbars in the same range, you can compare the curvatures by looking at the colours instead of the max/min values, too.

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:45:06Z ----------------------------------------------------------------

I think "inflexions" is still a bit vague -- if I remember high school correctly, a saddle point is also a point of inflection. With Gaussian curvature the principal curvatures are both the same sign so "extrema" or minimum/maximum might be better phrasing.

ojeda-e commented on 2021-09-02T08:25:34Z ----------------------------------------------------------------

I changed it to "concave regions".

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:46:05Z ----------------------------------------------------------------

inteprolation -> interpolation

lilyminium commented 3 years ago

A nice and thorough tutorial, @ojeda-e. I'll let @orbeckst answer the question about references, since I think it's his data. I mostly found some typos but my concrete feedback on the notebook was:

• I think you can swap more-itertools for numpy
• I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

ojeda-e commented 3 years ago

Thanks, @lilyminium for the comments. I will work on them and will replace itertools with numpy.

I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised Sign of mean curvature #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

You're right and I appreciate you double-check this. I actually made a (terrible) mistake due to a previous mental association I had with the bwr colormap. This is totally my bad. The sign is the opposite, soH>0 is valleys. The sign in the calculation of mean curvature is negative, same as in the mean curvature function in curvature.py. I'm very sorry I made this mistake and I will fix it in the next push (for this tutorial). Thanks again :)

ojeda-e commented 3 years ago

Thanks for this. I changed the name of the variables and used an approach following your suggestion. I didn't know about np.ediff1d, thanks!

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

This is a good point, thanks.

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed it to "concave regions".

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

---

View entire conversation on ReviewNB

orbeckst commented 3 years ago

I hope my comments on https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion actually show up...

orbeckst commented 3 years ago

Er, I am not sure how, but I commented on the notebook https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion but that's not linked to the PR?

Now I know how: I clicked on the "standalone notebook" link in NBreview as that was the most obvious thing to click on. Sorry, hope the comments are still useful.

ojeda-e commented 3 years ago

Thanks for the review @orbeckst. I can read your comments in the notebook although never got any notification, thanks for the link! I made a mistake by assuming the protein was fixed. Now in retrospect, I have no clue why did I even assume so. I will work on your comments. Thanks again!

ojeda-e commented 2 years ago

Apologies for the delay @orbeckst. The last push includes the suggested changes. I hope the changes are aligned with the ones you had in mind. I tried to follow all of them, please let me know if I missed anything there.

The more relevant changes are:

• Added discussion on protein diffusion (At the beginning and some other places).
• Added appendix to determine protein diffusion via MSD (Appendix 1).
• Added appendix with residue selection by leaflet. (Appendix 2).
• Updated function to generate NGL widget.

Thanks.

ojeda-e commented 2 years ago

Thanks @hmacdope I pushed the suggested changes in the tutorials.rst. However, I couldn't see your comments in ReviewNB. Would you please send me a link? This ReviewNB is very buggy 💀

hmacdope commented 2 years ago

@ojeda-e I have left comments on the notebook on the standalone review thingo at https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion

The tutorial looks and reads amazing! Mostly just little nitpicks and clarifications.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

diffuses

because the simulation was carried out without position restraints

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

NhaA-membrane simulation box (~120 Å x 120 Å divided into 6 x 6 = 36 bins). This warning can happen when the box size fluctuates as in an NPT simulation.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:55Z ----------------------------------------------------------------

diffuses through the membrane

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:56Z ----------------------------------------------------------------

diffuses through the membrane

OR

diffuses in the plane of the membrane

orbeckst commented 2 years ago

Looks great @ojeda-e ! I added minor comments on the notebook (which posted to the PR this time because I learned to comment on the diff).

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[lilyminium]

"an all atom MD simulations"

simulations -> simulation

"Then, we can select the phosphate atoms in our Universe with"

phosphate -> "phosphate phosphorus"

---

View entire conversation on ReviewNB

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:07:41Z ----------------------------------------------------------------

"The variable P_headgroups, however, include all the lipids in our system"

Instead of "include all the lipids", it might be better to be more explicit -- "include the phosphorus atoms of all the lipids in our system". Otherwise it sounds like it might be selecting other atoms in the lipid as well.

ojeda-e commented on 2021-09-02T06:26:24Z ----------------------------------------------------------------

This is a good point, thanks.

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:09:59Z ----------------------------------------------------------------

Wording suggestions:

"To identifying consecutive residue numbers in a list we can user consecutive_groups from the more_itertools package. We can save the residues in each leaflet in the membrane_indexes dictionary with:"

identifying -> identify

user -> use

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:17Z ----------------------------------------------------------------

"After identifying the residues in each leaflet, we can define lower and upper leaflet in our system with:"

Suggestion: After identifying the residues in each leaflet, we can define the lower and upper leaflet in our system with:

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:18Z ----------------------------------------------------------------

This is a nice thorough explanation, especially of the number of bins and coordinate wrapping!

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:19Z ----------------------------------------------------------------

Membranecurvature -> MembraneCurvature

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:20Z ----------------------------------------------------------------

MembraneCuvature -> MembraneCurvature

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:21Z ----------------------------------------------------------------

assymetric -> asymmetric

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:22Z ----------------------------------------------------------------

For this and every other image, it'd be really nice if the color map / color bar had the same range, so that yellow / blue meant the same thing in both plots. The trickiest one to do this for would be the surface. For the curvatures this is pretty simple, but for the surfaces I guess this could be solved by just setting the range to (25, 36) and (62, 73).

ojeda-e commented on 2021-09-02T08:28:05Z ----------------------------------------------------------------

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:24Z ----------------------------------------------------------------

Hmmm... this sounds odd to me. I think convention is to use the upward-pointing-normal, right, in which case positive H should be convex?

I wouldn't say null, I would explicitly say H=0 . Otherwise you risk conflating 0 with np.nan or None.

If you keep the colorbars in the same range, you can compare the curvatures by looking at the colours instead of the max/min values, too.

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:45:06Z ----------------------------------------------------------------

I think "inflexions" is still a bit vague -- if I remember high school correctly, a saddle point is also a point of inflection. With Gaussian curvature the principal curvatures are both the same sign so "extrema" or minimum/maximum might be better phrasing.

ojeda-e commented on 2021-09-02T08:25:34Z ----------------------------------------------------------------

I changed it to "concave regions".

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:46:05Z ----------------------------------------------------------------

inteprolation -> interpolation

lilyminium commented 3 years ago

A nice and thorough tutorial, @ojeda-e. I'll let @orbeckst answer the question about references, since I think it's his data. I mostly found some typos but my concrete feedback on the notebook was:

• I think you can swap more-itertools for numpy
• I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

ojeda-e commented 3 years ago

Thanks, @lilyminium for the comments. I will work on them and will replace itertools with numpy.

I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised Sign of mean curvature #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

You're right and I appreciate you double-check this. I actually made a (terrible) mistake due to a previous mental association I had with the bwr colormap. This is totally my bad. The sign is the opposite, soH>0 is valleys. The sign in the calculation of mean curvature is negative, same as in the mean curvature function in curvature.py. I'm very sorry I made this mistake and I will fix it in the next push (for this tutorial). Thanks again :)

ojeda-e commented 3 years ago

Thanks for this. I changed the name of the variables and used an approach following your suggestion. I didn't know about np.ediff1d, thanks!

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

This is a good point, thanks.

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed it to "concave regions".

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

---

View entire conversation on ReviewNB

orbeckst commented 3 years ago

I hope my comments on https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion actually show up...

orbeckst commented 3 years ago

Er, I am not sure how, but I commented on the notebook https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion but that's not linked to the PR?

Now I know how: I clicked on the "standalone notebook" link in NBreview as that was the most obvious thing to click on. Sorry, hope the comments are still useful.

ojeda-e commented 3 years ago

Thanks for the review @orbeckst. I can read your comments in the notebook although never got any notification, thanks for the link! I made a mistake by assuming the protein was fixed. Now in retrospect, I have no clue why did I even assume so. I will work on your comments. Thanks again!

ojeda-e commented 2 years ago

Apologies for the delay @orbeckst. The last push includes the suggested changes. I hope the changes are aligned with the ones you had in mind. I tried to follow all of them, please let me know if I missed anything there.

The more relevant changes are:

• Added discussion on protein diffusion (At the beginning and some other places).
• Added appendix to determine protein diffusion via MSD (Appendix 1).
• Added appendix with residue selection by leaflet. (Appendix 2).
• Updated function to generate NGL widget.

Thanks.

ojeda-e commented 2 years ago

Thanks @hmacdope I pushed the suggested changes in the tutorials.rst. However, I couldn't see your comments in ReviewNB. Would you please send me a link? This ReviewNB is very buggy 💀

hmacdope commented 2 years ago

@ojeda-e I have left comments on the notebook on the standalone review thingo at https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion

The tutorial looks and reads amazing! Mostly just little nitpicks and clarifications.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

diffuses

because the simulation was carried out without position restraints

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

NhaA-membrane simulation box (~120 Å x 120 Å divided into 6 x 6 = 36 bins). This warning can happen when the box size fluctuates as in an NPT simulation.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:55Z ----------------------------------------------------------------

diffuses through the membrane

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:56Z ----------------------------------------------------------------

diffuses through the membrane

OR

diffuses in the plane of the membrane

orbeckst commented 2 years ago

Looks great @ojeda-e ! I added minor comments on the notebook (which posted to the PR this time because I learned to comment on the diff).

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:09:59Z ----------------------------------------------------------------

Wording suggestions:

"To identifying consecutive residue numbers in a list we can user consecutive_groups from the more_itertools package. We can save the residues in each leaflet in the membrane_indexes dictionary with:"

identifying -> identify

user -> use

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:17Z ----------------------------------------------------------------

"After identifying the residues in each leaflet, we can define lower and upper leaflet in our system with:"

Suggestion: After identifying the residues in each leaflet, we can define the lower and upper leaflet in our system with:

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:18Z ----------------------------------------------------------------

This is a nice thorough explanation, especially of the number of bins and coordinate wrapping!

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:19Z ----------------------------------------------------------------

Membranecurvature -> MembraneCurvature

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:20Z ----------------------------------------------------------------

MembraneCuvature -> MembraneCurvature

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:21Z ----------------------------------------------------------------

assymetric -> asymmetric

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:22Z ----------------------------------------------------------------

For this and every other image, it'd be really nice if the color map / color bar had the same range, so that yellow / blue meant the same thing in both plots. The trickiest one to do this for would be the surface. For the curvatures this is pretty simple, but for the surfaces I guess this could be solved by just setting the range to (25, 36) and (62, 73).

ojeda-e commented on 2021-09-02T08:28:05Z ----------------------------------------------------------------

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:24Z ----------------------------------------------------------------

Hmmm... this sounds odd to me. I think convention is to use the upward-pointing-normal, right, in which case positive H should be convex?

I wouldn't say null, I would explicitly say H=0 . Otherwise you risk conflating 0 with np.nan or None.

If you keep the colorbars in the same range, you can compare the curvatures by looking at the colours instead of the max/min values, too.

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:45:06Z ----------------------------------------------------------------

I think "inflexions" is still a bit vague -- if I remember high school correctly, a saddle point is also a point of inflection. With Gaussian curvature the principal curvatures are both the same sign so "extrema" or minimum/maximum might be better phrasing.

ojeda-e commented on 2021-09-02T08:25:34Z ----------------------------------------------------------------

I changed it to "concave regions".

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:46:05Z ----------------------------------------------------------------

inteprolation -> interpolation

lilyminium commented 3 years ago

A nice and thorough tutorial, @ojeda-e. I'll let @orbeckst answer the question about references, since I think it's his data. I mostly found some typos but my concrete feedback on the notebook was:

• I think you can swap more-itertools for numpy
• I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

ojeda-e commented 3 years ago

Thanks, @lilyminium for the comments. I will work on them and will replace itertools with numpy.

I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised Sign of mean curvature #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

You're right and I appreciate you double-check this. I actually made a (terrible) mistake due to a previous mental association I had with the bwr colormap. This is totally my bad. The sign is the opposite, soH>0 is valleys. The sign in the calculation of mean curvature is negative, same as in the mean curvature function in curvature.py. I'm very sorry I made this mistake and I will fix it in the next push (for this tutorial). Thanks again :)

ojeda-e commented 3 years ago

Thanks for this. I changed the name of the variables and used an approach following your suggestion. I didn't know about np.ediff1d, thanks!

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

This is a good point, thanks.

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed it to "concave regions".

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

---

View entire conversation on ReviewNB

orbeckst commented 3 years ago

I hope my comments on https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion actually show up...

orbeckst commented 3 years ago

Er, I am not sure how, but I commented on the notebook https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion but that's not linked to the PR?

Now I know how: I clicked on the "standalone notebook" link in NBreview as that was the most obvious thing to click on. Sorry, hope the comments are still useful.

ojeda-e commented 3 years ago

Thanks for the review @orbeckst. I can read your comments in the notebook although never got any notification, thanks for the link! I made a mistake by assuming the protein was fixed. Now in retrospect, I have no clue why did I even assume so. I will work on your comments. Thanks again!

ojeda-e commented 2 years ago

Apologies for the delay @orbeckst. The last push includes the suggested changes. I hope the changes are aligned with the ones you had in mind. I tried to follow all of them, please let me know if I missed anything there.

The more relevant changes are:

• Added discussion on protein diffusion (At the beginning and some other places).
• Added appendix to determine protein diffusion via MSD (Appendix 1).
• Added appendix with residue selection by leaflet. (Appendix 2).
• Updated function to generate NGL widget.

Thanks.

ojeda-e commented 2 years ago

Thanks @hmacdope I pushed the suggested changes in the tutorials.rst. However, I couldn't see your comments in ReviewNB. Would you please send me a link? This ReviewNB is very buggy 💀

hmacdope commented 2 years ago

@ojeda-e I have left comments on the notebook on the standalone review thingo at https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion

The tutorial looks and reads amazing! Mostly just little nitpicks and clarifications.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

diffuses

because the simulation was carried out without position restraints

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

NhaA-membrane simulation box (~120 Å x 120 Å divided into 6 x 6 = 36 bins). This warning can happen when the box size fluctuates as in an NPT simulation.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:55Z ----------------------------------------------------------------

diffuses through the membrane

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:56Z ----------------------------------------------------------------

diffuses through the membrane

OR

diffuses in the plane of the membrane

orbeckst commented 2 years ago

Looks great @ojeda-e ! I added minor comments on the notebook (which posted to the PR this time because I learned to comment on the diff).

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:17Z ----------------------------------------------------------------

"After identifying the residues in each leaflet, we can define lower and upper leaflet in our system with:"

Suggestion: After identifying the residues in each leaflet, we can define the lower and upper leaflet in our system with:

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:18Z ----------------------------------------------------------------

This is a nice thorough explanation, especially of the number of bins and coordinate wrapping!

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:19Z ----------------------------------------------------------------

Membranecurvature -> MembraneCurvature

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:20Z ----------------------------------------------------------------

MembraneCuvature -> MembraneCurvature

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:21Z ----------------------------------------------------------------

assymetric -> asymmetric

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:22Z ----------------------------------------------------------------

For this and every other image, it'd be really nice if the color map / color bar had the same range, so that yellow / blue meant the same thing in both plots. The trickiest one to do this for would be the surface. For the curvatures this is pretty simple, but for the surfaces I guess this could be solved by just setting the range to (25, 36) and (62, 73).

ojeda-e commented on 2021-09-02T08:28:05Z ----------------------------------------------------------------

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:24Z ----------------------------------------------------------------

Hmmm... this sounds odd to me. I think convention is to use the upward-pointing-normal, right, in which case positive H should be convex?

I wouldn't say null, I would explicitly say H=0 . Otherwise you risk conflating 0 with np.nan or None.

If you keep the colorbars in the same range, you can compare the curvatures by looking at the colours instead of the max/min values, too.

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:45:06Z ----------------------------------------------------------------

I think "inflexions" is still a bit vague -- if I remember high school correctly, a saddle point is also a point of inflection. With Gaussian curvature the principal curvatures are both the same sign so "extrema" or minimum/maximum might be better phrasing.

ojeda-e commented on 2021-09-02T08:25:34Z ----------------------------------------------------------------

I changed it to "concave regions".

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:46:05Z ----------------------------------------------------------------

inteprolation -> interpolation

lilyminium commented 3 years ago

A nice and thorough tutorial, @ojeda-e. I'll let @orbeckst answer the question about references, since I think it's his data. I mostly found some typos but my concrete feedback on the notebook was:

• I think you can swap more-itertools for numpy
• I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

ojeda-e commented 3 years ago

Thanks, @lilyminium for the comments. I will work on them and will replace itertools with numpy.

I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised Sign of mean curvature #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

You're right and I appreciate you double-check this. I actually made a (terrible) mistake due to a previous mental association I had with the bwr colormap. This is totally my bad. The sign is the opposite, soH>0 is valleys. The sign in the calculation of mean curvature is negative, same as in the mean curvature function in curvature.py. I'm very sorry I made this mistake and I will fix it in the next push (for this tutorial). Thanks again :)

ojeda-e commented 3 years ago

Thanks for this. I changed the name of the variables and used an approach following your suggestion. I didn't know about np.ediff1d, thanks!

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

This is a good point, thanks.

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed it to "concave regions".

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

---

View entire conversation on ReviewNB

orbeckst commented 3 years ago

I hope my comments on https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion actually show up...

orbeckst commented 3 years ago

Er, I am not sure how, but I commented on the notebook https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion but that's not linked to the PR?

Now I know how: I clicked on the "standalone notebook" link in NBreview as that was the most obvious thing to click on. Sorry, hope the comments are still useful.

ojeda-e commented 3 years ago

Thanks for the review @orbeckst. I can read your comments in the notebook although never got any notification, thanks for the link! I made a mistake by assuming the protein was fixed. Now in retrospect, I have no clue why did I even assume so. I will work on your comments. Thanks again!

ojeda-e commented 2 years ago

Apologies for the delay @orbeckst. The last push includes the suggested changes. I hope the changes are aligned with the ones you had in mind. I tried to follow all of them, please let me know if I missed anything there.

The more relevant changes are:

• Added discussion on protein diffusion (At the beginning and some other places).
• Added appendix to determine protein diffusion via MSD (Appendix 1).
• Added appendix with residue selection by leaflet. (Appendix 2).
• Updated function to generate NGL widget.

Thanks.

ojeda-e commented 2 years ago

Thanks @hmacdope I pushed the suggested changes in the tutorials.rst. However, I couldn't see your comments in ReviewNB. Would you please send me a link? This ReviewNB is very buggy 💀

hmacdope commented 2 years ago

@ojeda-e I have left comments on the notebook on the standalone review thingo at https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion

The tutorial looks and reads amazing! Mostly just little nitpicks and clarifications.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

diffuses

because the simulation was carried out without position restraints

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

NhaA-membrane simulation box (~120 Å x 120 Å divided into 6 x 6 = 36 bins). This warning can happen when the box size fluctuates as in an NPT simulation.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:55Z ----------------------------------------------------------------

diffuses through the membrane

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:56Z ----------------------------------------------------------------

diffuses through the membrane

OR

diffuses in the plane of the membrane

orbeckst commented 2 years ago

Looks great @ojeda-e ! I added minor comments on the notebook (which posted to the PR this time because I learned to comment on the diff).

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:18Z ----------------------------------------------------------------

This is a nice thorough explanation, especially of the number of bins and coordinate wrapping!

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:19Z ----------------------------------------------------------------

Membranecurvature -> MembraneCurvature

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:20Z ----------------------------------------------------------------

MembraneCuvature -> MembraneCurvature

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:21Z ----------------------------------------------------------------

assymetric -> asymmetric

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:22Z ----------------------------------------------------------------

For this and every other image, it'd be really nice if the color map / color bar had the same range, so that yellow / blue meant the same thing in both plots. The trickiest one to do this for would be the surface. For the curvatures this is pretty simple, but for the surfaces I guess this could be solved by just setting the range to (25, 36) and (62, 73).

ojeda-e commented on 2021-09-02T08:28:05Z ----------------------------------------------------------------

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:24Z ----------------------------------------------------------------

Hmmm... this sounds odd to me. I think convention is to use the upward-pointing-normal, right, in which case positive H should be convex?

I wouldn't say null, I would explicitly say H=0 . Otherwise you risk conflating 0 with np.nan or None.

If you keep the colorbars in the same range, you can compare the curvatures by looking at the colours instead of the max/min values, too.

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:45:06Z ----------------------------------------------------------------

I think "inflexions" is still a bit vague -- if I remember high school correctly, a saddle point is also a point of inflection. With Gaussian curvature the principal curvatures are both the same sign so "extrema" or minimum/maximum might be better phrasing.

ojeda-e commented on 2021-09-02T08:25:34Z ----------------------------------------------------------------

I changed it to "concave regions".

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:46:05Z ----------------------------------------------------------------

inteprolation -> interpolation

lilyminium commented 3 years ago

A nice and thorough tutorial, @ojeda-e. I'll let @orbeckst answer the question about references, since I think it's his data. I mostly found some typos but my concrete feedback on the notebook was:

• I think you can swap more-itertools for numpy
• I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

ojeda-e commented 3 years ago

Thanks, @lilyminium for the comments. I will work on them and will replace itertools with numpy.

I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised Sign of mean curvature #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

You're right and I appreciate you double-check this. I actually made a (terrible) mistake due to a previous mental association I had with the bwr colormap. This is totally my bad. The sign is the opposite, soH>0 is valleys. The sign in the calculation of mean curvature is negative, same as in the mean curvature function in curvature.py. I'm very sorry I made this mistake and I will fix it in the next push (for this tutorial). Thanks again :)

ojeda-e commented 3 years ago

Thanks for this. I changed the name of the variables and used an approach following your suggestion. I didn't know about np.ediff1d, thanks!

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

This is a good point, thanks.

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed it to "concave regions".

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

---

View entire conversation on ReviewNB

orbeckst commented 3 years ago

I hope my comments on https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion actually show up...

orbeckst commented 3 years ago

Er, I am not sure how, but I commented on the notebook https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion but that's not linked to the PR?

Now I know how: I clicked on the "standalone notebook" link in NBreview as that was the most obvious thing to click on. Sorry, hope the comments are still useful.

ojeda-e commented 3 years ago

Thanks for the review @orbeckst. I can read your comments in the notebook although never got any notification, thanks for the link! I made a mistake by assuming the protein was fixed. Now in retrospect, I have no clue why did I even assume so. I will work on your comments. Thanks again!

ojeda-e commented 2 years ago

Apologies for the delay @orbeckst. The last push includes the suggested changes. I hope the changes are aligned with the ones you had in mind. I tried to follow all of them, please let me know if I missed anything there.

The more relevant changes are:

• Added discussion on protein diffusion (At the beginning and some other places).
• Added appendix to determine protein diffusion via MSD (Appendix 1).
• Added appendix with residue selection by leaflet. (Appendix 2).
• Updated function to generate NGL widget.

Thanks.

ojeda-e commented 2 years ago

Thanks @hmacdope I pushed the suggested changes in the tutorials.rst. However, I couldn't see your comments in ReviewNB. Would you please send me a link? This ReviewNB is very buggy 💀

hmacdope commented 2 years ago

@ojeda-e I have left comments on the notebook on the standalone review thingo at https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion

The tutorial looks and reads amazing! Mostly just little nitpicks and clarifications.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

diffuses

because the simulation was carried out without position restraints

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

NhaA-membrane simulation box (~120 Å x 120 Å divided into 6 x 6 = 36 bins). This warning can happen when the box size fluctuates as in an NPT simulation.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:55Z ----------------------------------------------------------------

diffuses through the membrane

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:56Z ----------------------------------------------------------------

diffuses through the membrane

OR

diffuses in the plane of the membrane

orbeckst commented 2 years ago

Looks great @ojeda-e ! I added minor comments on the notebook (which posted to the PR this time because I learned to comment on the diff).

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:19Z ----------------------------------------------------------------

Membranecurvature -> MembraneCurvature

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:20Z ----------------------------------------------------------------

MembraneCuvature -> MembraneCurvature

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:21Z ----------------------------------------------------------------

assymetric -> asymmetric

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:22Z ----------------------------------------------------------------

For this and every other image, it'd be really nice if the color map / color bar had the same range, so that yellow / blue meant the same thing in both plots. The trickiest one to do this for would be the surface. For the curvatures this is pretty simple, but for the surfaces I guess this could be solved by just setting the range to (25, 36) and (62, 73).

ojeda-e commented on 2021-09-02T08:28:05Z ----------------------------------------------------------------

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:24Z ----------------------------------------------------------------

Hmmm... this sounds odd to me. I think convention is to use the upward-pointing-normal, right, in which case positive H should be convex?

I wouldn't say null, I would explicitly say H=0 . Otherwise you risk conflating 0 with np.nan or None.

If you keep the colorbars in the same range, you can compare the curvatures by looking at the colours instead of the max/min values, too.

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:45:06Z ----------------------------------------------------------------

I think "inflexions" is still a bit vague -- if I remember high school correctly, a saddle point is also a point of inflection. With Gaussian curvature the principal curvatures are both the same sign so "extrema" or minimum/maximum might be better phrasing.

ojeda-e commented on 2021-09-02T08:25:34Z ----------------------------------------------------------------

I changed it to "concave regions".

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:46:05Z ----------------------------------------------------------------

inteprolation -> interpolation

lilyminium commented 3 years ago

A nice and thorough tutorial, @ojeda-e. I'll let @orbeckst answer the question about references, since I think it's his data. I mostly found some typos but my concrete feedback on the notebook was:

• I think you can swap more-itertools for numpy
• I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

ojeda-e commented 3 years ago

Thanks, @lilyminium for the comments. I will work on them and will replace itertools with numpy.

I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised Sign of mean curvature #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

You're right and I appreciate you double-check this. I actually made a (terrible) mistake due to a previous mental association I had with the bwr colormap. This is totally my bad. The sign is the opposite, soH>0 is valleys. The sign in the calculation of mean curvature is negative, same as in the mean curvature function in curvature.py. I'm very sorry I made this mistake and I will fix it in the next push (for this tutorial). Thanks again :)

ojeda-e commented 3 years ago

Thanks for this. I changed the name of the variables and used an approach following your suggestion. I didn't know about np.ediff1d, thanks!

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

This is a good point, thanks.

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed it to "concave regions".

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

---

View entire conversation on ReviewNB

orbeckst commented 3 years ago

I hope my comments on https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion actually show up...

orbeckst commented 3 years ago

Er, I am not sure how, but I commented on the notebook https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion but that's not linked to the PR?

Now I know how: I clicked on the "standalone notebook" link in NBreview as that was the most obvious thing to click on. Sorry, hope the comments are still useful.

ojeda-e commented 3 years ago

Thanks for the review @orbeckst. I can read your comments in the notebook although never got any notification, thanks for the link! I made a mistake by assuming the protein was fixed. Now in retrospect, I have no clue why did I even assume so. I will work on your comments. Thanks again!

ojeda-e commented 2 years ago

Apologies for the delay @orbeckst. The last push includes the suggested changes. I hope the changes are aligned with the ones you had in mind. I tried to follow all of them, please let me know if I missed anything there.

The more relevant changes are:

• Added discussion on protein diffusion (At the beginning and some other places).
• Added appendix to determine protein diffusion via MSD (Appendix 1).
• Added appendix with residue selection by leaflet. (Appendix 2).
• Updated function to generate NGL widget.

Thanks.

ojeda-e commented 2 years ago

Thanks @hmacdope I pushed the suggested changes in the tutorials.rst. However, I couldn't see your comments in ReviewNB. Would you please send me a link? This ReviewNB is very buggy 💀

hmacdope commented 2 years ago

@ojeda-e I have left comments on the notebook on the standalone review thingo at https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion

The tutorial looks and reads amazing! Mostly just little nitpicks and clarifications.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

diffuses

because the simulation was carried out without position restraints

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

NhaA-membrane simulation box (~120 Å x 120 Å divided into 6 x 6 = 36 bins). This warning can happen when the box size fluctuates as in an NPT simulation.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:55Z ----------------------------------------------------------------

diffuses through the membrane

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:56Z ----------------------------------------------------------------

diffuses through the membrane

OR

diffuses in the plane of the membrane

orbeckst commented 2 years ago

Looks great @ojeda-e ! I added minor comments on the notebook (which posted to the PR this time because I learned to comment on the diff).

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:20Z ----------------------------------------------------------------

MembraneCuvature -> MembraneCurvature

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:21Z ----------------------------------------------------------------

assymetric -> asymmetric

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:22Z ----------------------------------------------------------------

For this and every other image, it'd be really nice if the color map / color bar had the same range, so that yellow / blue meant the same thing in both plots. The trickiest one to do this for would be the surface. For the curvatures this is pretty simple, but for the surfaces I guess this could be solved by just setting the range to (25, 36) and (62, 73).

ojeda-e commented on 2021-09-02T08:28:05Z ----------------------------------------------------------------

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:24Z ----------------------------------------------------------------

Hmmm... this sounds odd to me. I think convention is to use the upward-pointing-normal, right, in which case positive H should be convex?

I wouldn't say null, I would explicitly say H=0 . Otherwise you risk conflating 0 with np.nan or None.

If you keep the colorbars in the same range, you can compare the curvatures by looking at the colours instead of the max/min values, too.

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:45:06Z ----------------------------------------------------------------

I think "inflexions" is still a bit vague -- if I remember high school correctly, a saddle point is also a point of inflection. With Gaussian curvature the principal curvatures are both the same sign so "extrema" or minimum/maximum might be better phrasing.

ojeda-e commented on 2021-09-02T08:25:34Z ----------------------------------------------------------------

I changed it to "concave regions".

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:46:05Z ----------------------------------------------------------------

inteprolation -> interpolation

lilyminium commented 3 years ago

A nice and thorough tutorial, @ojeda-e. I'll let @orbeckst answer the question about references, since I think it's his data. I mostly found some typos but my concrete feedback on the notebook was:

• I think you can swap more-itertools for numpy
• I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

ojeda-e commented 3 years ago

Thanks, @lilyminium for the comments. I will work on them and will replace itertools with numpy.

I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised Sign of mean curvature #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

You're right and I appreciate you double-check this. I actually made a (terrible) mistake due to a previous mental association I had with the bwr colormap. This is totally my bad. The sign is the opposite, soH>0 is valleys. The sign in the calculation of mean curvature is negative, same as in the mean curvature function in curvature.py. I'm very sorry I made this mistake and I will fix it in the next push (for this tutorial). Thanks again :)

ojeda-e commented 3 years ago

Thanks for this. I changed the name of the variables and used an approach following your suggestion. I didn't know about np.ediff1d, thanks!

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

This is a good point, thanks.

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed it to "concave regions".

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

---

View entire conversation on ReviewNB

orbeckst commented 3 years ago

I hope my comments on https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion actually show up...

orbeckst commented 3 years ago

Er, I am not sure how, but I commented on the notebook https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion but that's not linked to the PR?

Now I know how: I clicked on the "standalone notebook" link in NBreview as that was the most obvious thing to click on. Sorry, hope the comments are still useful.

ojeda-e commented 3 years ago

Thanks for the review @orbeckst. I can read your comments in the notebook although never got any notification, thanks for the link! I made a mistake by assuming the protein was fixed. Now in retrospect, I have no clue why did I even assume so. I will work on your comments. Thanks again!

ojeda-e commented 2 years ago

Apologies for the delay @orbeckst. The last push includes the suggested changes. I hope the changes are aligned with the ones you had in mind. I tried to follow all of them, please let me know if I missed anything there.

The more relevant changes are:

• Added discussion on protein diffusion (At the beginning and some other places).
• Added appendix to determine protein diffusion via MSD (Appendix 1).
• Added appendix with residue selection by leaflet. (Appendix 2).
• Updated function to generate NGL widget.

Thanks.

ojeda-e commented 2 years ago

Thanks @hmacdope I pushed the suggested changes in the tutorials.rst. However, I couldn't see your comments in ReviewNB. Would you please send me a link? This ReviewNB is very buggy 💀

hmacdope commented 2 years ago

@ojeda-e I have left comments on the notebook on the standalone review thingo at https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion

The tutorial looks and reads amazing! Mostly just little nitpicks and clarifications.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

diffuses

because the simulation was carried out without position restraints

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

NhaA-membrane simulation box (~120 Å x 120 Å divided into 6 x 6 = 36 bins). This warning can happen when the box size fluctuates as in an NPT simulation.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:55Z ----------------------------------------------------------------

diffuses through the membrane

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:56Z ----------------------------------------------------------------

diffuses through the membrane

OR

diffuses in the plane of the membrane

orbeckst commented 2 years ago

Looks great @ojeda-e ! I added minor comments on the notebook (which posted to the PR this time because I learned to comment on the diff).

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:21Z ----------------------------------------------------------------

assymetric -> asymmetric

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:22Z ----------------------------------------------------------------

For this and every other image, it'd be really nice if the color map / color bar had the same range, so that yellow / blue meant the same thing in both plots. The trickiest one to do this for would be the surface. For the curvatures this is pretty simple, but for the surfaces I guess this could be solved by just setting the range to (25, 36) and (62, 73).

ojeda-e commented on 2021-09-02T08:28:05Z ----------------------------------------------------------------

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:24Z ----------------------------------------------------------------

Hmmm... this sounds odd to me. I think convention is to use the upward-pointing-normal, right, in which case positive H should be convex?

I wouldn't say null, I would explicitly say H=0 . Otherwise you risk conflating 0 with np.nan or None.

If you keep the colorbars in the same range, you can compare the curvatures by looking at the colours instead of the max/min values, too.

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:45:06Z ----------------------------------------------------------------

I think "inflexions" is still a bit vague -- if I remember high school correctly, a saddle point is also a point of inflection. With Gaussian curvature the principal curvatures are both the same sign so "extrema" or minimum/maximum might be better phrasing.

ojeda-e commented on 2021-09-02T08:25:34Z ----------------------------------------------------------------

I changed it to "concave regions".

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:46:05Z ----------------------------------------------------------------

inteprolation -> interpolation

lilyminium commented 3 years ago

A nice and thorough tutorial, @ojeda-e. I'll let @orbeckst answer the question about references, since I think it's his data. I mostly found some typos but my concrete feedback on the notebook was:

• I think you can swap more-itertools for numpy
• I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

ojeda-e commented 3 years ago

Thanks, @lilyminium for the comments. I will work on them and will replace itertools with numpy.

I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised Sign of mean curvature #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

You're right and I appreciate you double-check this. I actually made a (terrible) mistake due to a previous mental association I had with the bwr colormap. This is totally my bad. The sign is the opposite, soH>0 is valleys. The sign in the calculation of mean curvature is negative, same as in the mean curvature function in curvature.py. I'm very sorry I made this mistake and I will fix it in the next push (for this tutorial). Thanks again :)

ojeda-e commented 3 years ago

Thanks for this. I changed the name of the variables and used an approach following your suggestion. I didn't know about np.ediff1d, thanks!

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

This is a good point, thanks.

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed it to "concave regions".

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

---

View entire conversation on ReviewNB

orbeckst commented 3 years ago

I hope my comments on https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion actually show up...

orbeckst commented 3 years ago

Er, I am not sure how, but I commented on the notebook https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion but that's not linked to the PR?

Now I know how: I clicked on the "standalone notebook" link in NBreview as that was the most obvious thing to click on. Sorry, hope the comments are still useful.

ojeda-e commented 3 years ago

Thanks for the review @orbeckst. I can read your comments in the notebook although never got any notification, thanks for the link! I made a mistake by assuming the protein was fixed. Now in retrospect, I have no clue why did I even assume so. I will work on your comments. Thanks again!

ojeda-e commented 2 years ago

Apologies for the delay @orbeckst. The last push includes the suggested changes. I hope the changes are aligned with the ones you had in mind. I tried to follow all of them, please let me know if I missed anything there.

The more relevant changes are:

• Added discussion on protein diffusion (At the beginning and some other places).
• Added appendix to determine protein diffusion via MSD (Appendix 1).
• Added appendix with residue selection by leaflet. (Appendix 2).
• Updated function to generate NGL widget.

Thanks.

ojeda-e commented 2 years ago

Thanks @hmacdope I pushed the suggested changes in the tutorials.rst. However, I couldn't see your comments in ReviewNB. Would you please send me a link? This ReviewNB is very buggy 💀

hmacdope commented 2 years ago

@ojeda-e I have left comments on the notebook on the standalone review thingo at https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion

The tutorial looks and reads amazing! Mostly just little nitpicks and clarifications.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

diffuses

because the simulation was carried out without position restraints

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

NhaA-membrane simulation box (~120 Å x 120 Å divided into 6 x 6 = 36 bins). This warning can happen when the box size fluctuates as in an NPT simulation.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:55Z ----------------------------------------------------------------

diffuses through the membrane

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:56Z ----------------------------------------------------------------

diffuses through the membrane

OR

diffuses in the plane of the membrane

orbeckst commented 2 years ago

Looks great @ojeda-e ! I added minor comments on the notebook (which posted to the PR this time because I learned to comment on the diff).

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:22Z ----------------------------------------------------------------

For this and every other image, it'd be really nice if the color map / color bar had the same range, so that yellow / blue meant the same thing in both plots. The trickiest one to do this for would be the surface. For the curvatures this is pretty simple, but for the surfaces I guess this could be solved by just setting the range to (25, 36) and (62, 73).

ojeda-e commented on 2021-09-02T08:28:05Z ----------------------------------------------------------------

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:24Z ----------------------------------------------------------------

Hmmm... this sounds odd to me. I think convention is to use the upward-pointing-normal, right, in which case positive H should be convex?

I wouldn't say null, I would explicitly say H=0 . Otherwise you risk conflating 0 with np.nan or None.

If you keep the colorbars in the same range, you can compare the curvatures by looking at the colours instead of the max/min values, too.

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:45:06Z ----------------------------------------------------------------

I think "inflexions" is still a bit vague -- if I remember high school correctly, a saddle point is also a point of inflection. With Gaussian curvature the principal curvatures are both the same sign so "extrema" or minimum/maximum might be better phrasing.

ojeda-e commented on 2021-09-02T08:25:34Z ----------------------------------------------------------------

I changed it to "concave regions".

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:46:05Z ----------------------------------------------------------------

inteprolation -> interpolation

lilyminium commented 3 years ago

A nice and thorough tutorial, @ojeda-e. I'll let @orbeckst answer the question about references, since I think it's his data. I mostly found some typos but my concrete feedback on the notebook was:

• I think you can swap more-itertools for numpy
• I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

ojeda-e commented 3 years ago

Thanks, @lilyminium for the comments. I will work on them and will replace itertools with numpy.

I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised Sign of mean curvature #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

You're right and I appreciate you double-check this. I actually made a (terrible) mistake due to a previous mental association I had with the bwr colormap. This is totally my bad. The sign is the opposite, soH>0 is valleys. The sign in the calculation of mean curvature is negative, same as in the mean curvature function in curvature.py. I'm very sorry I made this mistake and I will fix it in the next push (for this tutorial). Thanks again :)

ojeda-e commented 3 years ago

Thanks for this. I changed the name of the variables and used an approach following your suggestion. I didn't know about np.ediff1d, thanks!

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

This is a good point, thanks.

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed it to "concave regions".

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

---

View entire conversation on ReviewNB

orbeckst commented 3 years ago

I hope my comments on https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion actually show up...

orbeckst commented 3 years ago

Er, I am not sure how, but I commented on the notebook https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion but that's not linked to the PR?

Now I know how: I clicked on the "standalone notebook" link in NBreview as that was the most obvious thing to click on. Sorry, hope the comments are still useful.

ojeda-e commented 3 years ago

Thanks for the review @orbeckst. I can read your comments in the notebook although never got any notification, thanks for the link! I made a mistake by assuming the protein was fixed. Now in retrospect, I have no clue why did I even assume so. I will work on your comments. Thanks again!

ojeda-e commented 2 years ago

Apologies for the delay @orbeckst. The last push includes the suggested changes. I hope the changes are aligned with the ones you had in mind. I tried to follow all of them, please let me know if I missed anything there.

The more relevant changes are:

• Added discussion on protein diffusion (At the beginning and some other places).
• Added appendix to determine protein diffusion via MSD (Appendix 1).
• Added appendix with residue selection by leaflet. (Appendix 2).
• Updated function to generate NGL widget.

Thanks.

ojeda-e commented 2 years ago

Thanks @hmacdope I pushed the suggested changes in the tutorials.rst. However, I couldn't see your comments in ReviewNB. Would you please send me a link? This ReviewNB is very buggy 💀

hmacdope commented 2 years ago

@ojeda-e I have left comments on the notebook on the standalone review thingo at https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion

The tutorial looks and reads amazing! Mostly just little nitpicks and clarifications.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

diffuses

because the simulation was carried out without position restraints

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

NhaA-membrane simulation box (~120 Å x 120 Å divided into 6 x 6 = 36 bins). This warning can happen when the box size fluctuates as in an NPT simulation.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:55Z ----------------------------------------------------------------

diffuses through the membrane

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:56Z ----------------------------------------------------------------

diffuses through the membrane

OR

diffuses in the plane of the membrane

orbeckst commented 2 years ago

Looks great @ojeda-e ! I added minor comments on the notebook (which posted to the PR this time because I learned to comment on the diff).

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:35:24Z ----------------------------------------------------------------

Hmmm... this sounds odd to me. I think convention is to use the upward-pointing-normal, right, in which case positive H should be convex?

I wouldn't say null, I would explicitly say H=0 . Otherwise you risk conflating 0 with np.nan or None.

If you keep the colorbars in the same range, you can compare the curvatures by looking at the colours instead of the max/min values, too.

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:45:06Z ----------------------------------------------------------------

I think "inflexions" is still a bit vague -- if I remember high school correctly, a saddle point is also a point of inflection. With Gaussian curvature the principal curvatures are both the same sign so "extrema" or minimum/maximum might be better phrasing.

ojeda-e commented on 2021-09-02T08:25:34Z ----------------------------------------------------------------

I changed it to "concave regions".

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:46:05Z ----------------------------------------------------------------

inteprolation -> interpolation

lilyminium commented 3 years ago

A nice and thorough tutorial, @ojeda-e. I'll let @orbeckst answer the question about references, since I think it's his data. I mostly found some typos but my concrete feedback on the notebook was:

• I think you can swap more-itertools for numpy
• I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

ojeda-e commented 3 years ago

Thanks, @lilyminium for the comments. I will work on them and will replace itertools with numpy.

I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised Sign of mean curvature #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

You're right and I appreciate you double-check this. I actually made a (terrible) mistake due to a previous mental association I had with the bwr colormap. This is totally my bad. The sign is the opposite, soH>0 is valleys. The sign in the calculation of mean curvature is negative, same as in the mean curvature function in curvature.py. I'm very sorry I made this mistake and I will fix it in the next push (for this tutorial). Thanks again :)

ojeda-e commented 3 years ago

Thanks for this. I changed the name of the variables and used an approach following your suggestion. I didn't know about np.ediff1d, thanks!

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

This is a good point, thanks.

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed it to "concave regions".

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

---

View entire conversation on ReviewNB

orbeckst commented 3 years ago

I hope my comments on https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion actually show up...

orbeckst commented 3 years ago

Er, I am not sure how, but I commented on the notebook https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion but that's not linked to the PR?

Now I know how: I clicked on the "standalone notebook" link in NBreview as that was the most obvious thing to click on. Sorry, hope the comments are still useful.

ojeda-e commented 3 years ago

Thanks for the review @orbeckst. I can read your comments in the notebook although never got any notification, thanks for the link! I made a mistake by assuming the protein was fixed. Now in retrospect, I have no clue why did I even assume so. I will work on your comments. Thanks again!

ojeda-e commented 2 years ago

Apologies for the delay @orbeckst. The last push includes the suggested changes. I hope the changes are aligned with the ones you had in mind. I tried to follow all of them, please let me know if I missed anything there.

The more relevant changes are:

• Added discussion on protein diffusion (At the beginning and some other places).
• Added appendix to determine protein diffusion via MSD (Appendix 1).
• Added appendix with residue selection by leaflet. (Appendix 2).
• Updated function to generate NGL widget.

Thanks.

ojeda-e commented 2 years ago

Thanks @hmacdope I pushed the suggested changes in the tutorials.rst. However, I couldn't see your comments in ReviewNB. Would you please send me a link? This ReviewNB is very buggy 💀

hmacdope commented 2 years ago

@ojeda-e I have left comments on the notebook on the standalone review thingo at https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion

The tutorial looks and reads amazing! Mostly just little nitpicks and clarifications.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

diffuses

because the simulation was carried out without position restraints

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

NhaA-membrane simulation box (~120 Å x 120 Å divided into 6 x 6 = 36 bins). This warning can happen when the box size fluctuates as in an NPT simulation.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:55Z ----------------------------------------------------------------

diffuses through the membrane

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:56Z ----------------------------------------------------------------

diffuses through the membrane

OR

diffuses in the plane of the membrane

orbeckst commented 2 years ago

Looks great @ojeda-e ! I added minor comments on the notebook (which posted to the PR this time because I learned to comment on the diff).

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:45:06Z ----------------------------------------------------------------

I think "inflexions" is still a bit vague -- if I remember high school correctly, a saddle point is also a point of inflection. With Gaussian curvature the principal curvatures are both the same sign so "extrema" or minimum/maximum might be better phrasing.

ojeda-e commented on 2021-09-02T08:25:34Z ----------------------------------------------------------------

I changed it to "concave regions".

review-notebook-app[bot] commented 3 years ago

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:46:05Z ----------------------------------------------------------------

inteprolation -> interpolation

lilyminium commented 3 years ago

A nice and thorough tutorial, @ojeda-e. I'll let @orbeckst answer the question about references, since I think it's his data. I mostly found some typos but my concrete feedback on the notebook was:

• I think you can swap more-itertools for numpy
• I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

ojeda-e commented 3 years ago

Thanks, @lilyminium for the comments. I will work on them and will replace itertools with numpy.

I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised Sign of mean curvature #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

You're right and I appreciate you double-check this. I actually made a (terrible) mistake due to a previous mental association I had with the bwr colormap. This is totally my bad. The sign is the opposite, soH>0 is valleys. The sign in the calculation of mean curvature is negative, same as in the mean curvature function in curvature.py. I'm very sorry I made this mistake and I will fix it in the next push (for this tutorial). Thanks again :)

ojeda-e commented 3 years ago

Thanks for this. I changed the name of the variables and used an approach following your suggestion. I didn't know about np.ediff1d, thanks!

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

This is a good point, thanks.

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed it to "concave regions".

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

---

View entire conversation on ReviewNB

orbeckst commented 3 years ago

I hope my comments on https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion actually show up...

orbeckst commented 3 years ago

Er, I am not sure how, but I commented on the notebook https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion but that's not linked to the PR?

Now I know how: I clicked on the "standalone notebook" link in NBreview as that was the most obvious thing to click on. Sorry, hope the comments are still useful.

ojeda-e commented 3 years ago

Thanks for the review @orbeckst. I can read your comments in the notebook although never got any notification, thanks for the link! I made a mistake by assuming the protein was fixed. Now in retrospect, I have no clue why did I even assume so. I will work on your comments. Thanks again!

ojeda-e commented 2 years ago

Apologies for the delay @orbeckst. The last push includes the suggested changes. I hope the changes are aligned with the ones you had in mind. I tried to follow all of them, please let me know if I missed anything there.

The more relevant changes are:

• Added discussion on protein diffusion (At the beginning and some other places).
• Added appendix to determine protein diffusion via MSD (Appendix 1).
• Added appendix with residue selection by leaflet. (Appendix 2).
• Updated function to generate NGL widget.

Thanks.

ojeda-e commented 2 years ago

Thanks @hmacdope I pushed the suggested changes in the tutorials.rst. However, I couldn't see your comments in ReviewNB. Would you please send me a link? This ReviewNB is very buggy 💀

hmacdope commented 2 years ago

@ojeda-e I have left comments on the notebook on the standalone review thingo at https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion

The tutorial looks and reads amazing! Mostly just little nitpicks and clarifications.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

diffuses

because the simulation was carried out without position restraints

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

NhaA-membrane simulation box (~120 Å x 120 Å divided into 6 x 6 = 36 bins). This warning can happen when the box size fluctuates as in an NPT simulation.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:55Z ----------------------------------------------------------------

diffuses through the membrane

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:56Z ----------------------------------------------------------------

diffuses through the membrane

OR

diffuses in the plane of the membrane

orbeckst commented 2 years ago

Looks great @ojeda-e ! I added minor comments on the notebook (which posted to the PR this time because I learned to comment on the diff).

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

lilyminium commented on 2021-08-31T18:46:05Z ----------------------------------------------------------------

inteprolation -> interpolation

lilyminium commented 3 years ago

A nice and thorough tutorial, @ojeda-e. I'll let @orbeckst answer the question about references, since I think it's his data. I mostly found some typos but my concrete feedback on the notebook was:

• I think you can swap more-itertools for numpy
• I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

ojeda-e commented 3 years ago

Thanks, @lilyminium for the comments. I will work on them and will replace itertools with numpy.

I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised Sign of mean curvature #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

You're right and I appreciate you double-check this. I actually made a (terrible) mistake due to a previous mental association I had with the bwr colormap. This is totally my bad. The sign is the opposite, soH>0 is valleys. The sign in the calculation of mean curvature is negative, same as in the mean curvature function in curvature.py. I'm very sorry I made this mistake and I will fix it in the next push (for this tutorial). Thanks again :)

ojeda-e commented 3 years ago

Thanks for this. I changed the name of the variables and used an approach following your suggestion. I didn't know about np.ediff1d, thanks!

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

This is a good point, thanks.

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed it to "concave regions".

---

View entire conversation on ReviewNB

ojeda-e commented 3 years ago

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

---

View entire conversation on ReviewNB

orbeckst commented 3 years ago

I hope my comments on https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion actually show up...

orbeckst commented 3 years ago

Er, I am not sure how, but I commented on the notebook https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion but that's not linked to the PR?

Now I know how: I clicked on the "standalone notebook" link in NBreview as that was the most obvious thing to click on. Sorry, hope the comments are still useful.

ojeda-e commented 3 years ago

Thanks for the review @orbeckst. I can read your comments in the notebook although never got any notification, thanks for the link! I made a mistake by assuming the protein was fixed. Now in retrospect, I have no clue why did I even assume so. I will work on your comments. Thanks again!

ojeda-e commented 2 years ago

Apologies for the delay @orbeckst. The last push includes the suggested changes. I hope the changes are aligned with the ones you had in mind. I tried to follow all of them, please let me know if I missed anything there.

The more relevant changes are:

• Added discussion on protein diffusion (At the beginning and some other places).
• Added appendix to determine protein diffusion via MSD (Appendix 1).
• Added appendix with residue selection by leaflet. (Appendix 2).
• Updated function to generate NGL widget.

Thanks.

ojeda-e commented 2 years ago

Thanks @hmacdope I pushed the suggested changes in the tutorials.rst. However, I couldn't see your comments in ReviewNB. Would you please send me a link? This ReviewNB is very buggy 💀

hmacdope commented 2 years ago

@ojeda-e I have left comments on the notebook on the standalone review thingo at https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion

The tutorial looks and reads amazing! Mostly just little nitpicks and clarifications.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

diffuses

because the simulation was carried out without position restraints

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

NhaA-membrane simulation box (~120 Å x 120 Å divided into 6 x 6 = 36 bins). This warning can happen when the box size fluctuates as in an NPT simulation.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:55Z ----------------------------------------------------------------

diffuses through the membrane

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:56Z ----------------------------------------------------------------

diffuses through the membrane

OR

diffuses in the plane of the membrane

orbeckst commented 2 years ago

Looks great @ojeda-e ! I added minor comments on the notebook (which posted to the PR this time because I learned to comment on the diff).

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[lilyminium]

A nice and thorough tutorial, @ojeda-e. I'll let @orbeckst answer the question about references, since I think it's his data. I mostly found some typos but my concrete feedback on the notebook was:

• I think you can swap more-itertools for numpy
• I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

[ojeda-e]

Thanks, @lilyminium for the comments. I will work on them and will replace itertools with numpy.

I think the sign of the mean curvature is reversed from convention, i.e. is using the downward pointing z-axis as the normal. This isn't a documentation issue so much as a code / science one, so I raised Sign of mean curvature #70. However, you are likely more familiar with membrane literature than I am -- is it the new convention in membrane MD that positive mean curvature corresponds with peaks?

You're right and I appreciate you double-check this. I actually made a (terrible) mistake due to a previous mental association I had with the bwr colormap. This is totally my bad. The sign is the opposite, soH>0 is valleys. The sign in the calculation of mean curvature is negative, same as in the mean curvature function in curvature.py. I'm very sorry I made this mistake and I will fix it in the next push (for this tutorial). Thanks again :)

[ojeda-e]

Thanks for this. I changed the name of the variables and used an approach following your suggestion. I didn't know about np.ediff1d, thanks!

---

View entire conversation on ReviewNB

[ojeda-e]

This is a good point, thanks.

---

View entire conversation on ReviewNB

[ojeda-e]

I changed it to "concave regions".

---

View entire conversation on ReviewNB

[ojeda-e]

I changed the limits for each plot although the surface one goes from (26,33) and (63,72).

---

View entire conversation on ReviewNB

[orbeckst]

I hope my comments on https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion actually show up...

[orbeckst]

Er, I am not sure how, but I commented on the notebook https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion but that's not linked to the PR?

Now I know how: I clicked on the "standalone notebook" link in NBreview as that was the most obvious thing to click on. Sorry, hope the comments are still useful.

[ojeda-e]

Thanks for the review @orbeckst. I can read your comments in the notebook although never got any notification, thanks for the link! I made a mistake by assuming the protein was fixed. Now in retrospect, I have no clue why did I even assume so. I will work on your comments. Thanks again!

[ojeda-e]

Apologies for the delay @orbeckst. The last push includes the suggested changes. I hope the changes are aligned with the ones you had in mind. I tried to follow all of them, please let me know if I missed anything there.

The more relevant changes are:

• Added discussion on protein diffusion (At the beginning and some other places).
• Added appendix to determine protein diffusion via MSD (Appendix 1).
• Added appendix with residue selection by leaflet. (Appendix 2).
• Updated function to generate NGL widget.

Thanks.

[ojeda-e]

Thanks @hmacdope I pushed the suggested changes in the tutorials.rst. However, I couldn't see your comments in ReviewNB. Would you please send me a link? This ReviewNB is very buggy 💀

[hmacdope]

@ojeda-e I have left comments on the notebook on the standalone review thingo at https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue68/docs%2Fsource%2Fpages%2FCurvature_membrane_protein_pr_all_atom.ipynb%2F/discussion

The tutorial looks and reads amazing! Mostly just little nitpicks and clarifications.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

diffuses

because the simulation was carried out without position restraints

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

NhaA-membrane simulation box (~120 Å x 120 Å divided into 6 x 6 = 36 bins). This warning can happen when the box size fluctuates as in an NPT simulation.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:55Z ----------------------------------------------------------------

diffuses through the membrane

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:56Z ----------------------------------------------------------------

diffuses through the membrane

OR

diffuses in the plane of the membrane

orbeckst commented 2 years ago

Looks great @ojeda-e ! I added minor comments on the notebook (which posted to the PR this time because I learned to comment on the diff).

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:54Z ----------------------------------------------------------------

NhaA-membrane simulation box (~120 Å x 120 Å divided into 6 x 6 = 36 bins). This warning can happen when the box size fluctuates as in an NPT simulation.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:55Z ----------------------------------------------------------------

diffuses through the membrane

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:56Z ----------------------------------------------------------------

diffuses through the membrane

OR

diffuses in the plane of the membrane

orbeckst commented 2 years ago

Looks great @ojeda-e ! I added minor comments on the notebook (which posted to the PR this time because I learned to comment on the diff).

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:55Z ----------------------------------------------------------------

diffuses through the membrane

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:56Z ----------------------------------------------------------------

diffuses through the membrane

OR

diffuses in the plane of the membrane

orbeckst commented 2 years ago

Looks great @ojeda-e ! I added minor comments on the notebook (which posted to the PR this time because I learned to comment on the diff).

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

orbeckst commented on 2021-09-29T17:26:56Z ----------------------------------------------------------------

diffuses through the membrane

OR

diffuses in the plane of the membrane

orbeckst commented 2 years ago

Looks great @ojeda-e ! I added minor comments on the notebook (which posted to the PR this time because I learned to comment on the diff).

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[orbeckst]

Looks great @ojeda-e ! I added minor comments on the notebook (which posted to the PR this time because I learned to comment on the diff).