Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

updated protein 1 tutorial #40

Conversation

csbrasnett
Copy link
Contributor

One user over in the discussion forum has had issues with working out the input string for secondary structure specification. From my memory, the previous version of the tutorial had -dssp as the default argument, but I guess when it was revised that was changed for people who didn't have it installed?

However, I'm not sure if how it's been revised is particularly clear for people who aren't already familiar with martinize2 functionality, so I've tried having a go at a small rewrite to hopefully reintroduce some clarity. I realise this is a matter of personal opinion, so please let me know what you think!

@csbrasnett csbrasnett changed the title updated proteion 1 tutorial updated protein 1 tutorial Sep 23, 2024
@Lp0lp
Copy link
Contributor

Lp0lp commented Sep 24, 2024

Heyo! We've talked about reviewing a few tutorials and this was definitely one of them. Especially because a lot of it is now deprecated (the entire gomodel routine). If we are fixing part of it I would suggest we also fix this bit?

If you're interested @csbrasnett you could have a go at it, or I can also have a look in the next few days. Basically making the tutorial in line with the GoMartini paper, using martinize instead of the creategovirt.py script.

I would say we could then have Paulo @paulocts , for example, give it a read through before we merge this. What do you think?

@csbrasnett
Copy link
Contributor Author

Ah nice! Yep happy to have a look at the rest of the tutorial as well so we get this done in one PR. I’ll have a look in the next few days and push it so you guys can have a look

@csbrasnett
Copy link
Contributor Author

@Lp0lp let me know what you think about that. It occurs to me that it might be useful to have a section here about OLIVES, but I'm not sure I'd want to write that ahead of @KasperBuskPedersen ?

@KasperBuskPedersen
Copy link

@csbrasnett please go ahead and write a small section of how to set up OLIVES if you find that useful. You can basically just copy paste from the OLIVES github. Alternatively just write that OLIVES is an alternative and link the github? Unfortunately I don't have time to write or test that atm.

@csbrasnett
Copy link
Contributor Author

I think it'd be nice to have everything in one place as much as possible, I'll try to cook something up based on what you've done and also link through for more examples with the varying quaternary structure interaction strength so we don't overcomplicate too.

This is defo out of scope atm, but is there any desire to implement olives into martinize2 at some point? I realise it'd be a lot of work, but it might be nice in terms of workflow optimisation? I'd definitely appreciate it for one (and would be up for trying to do it when I have some time)!

@KasperBuskPedersen
Copy link

@csbrasnett Actually, I think there is a risk of doing new users a disservice by automating it too much. Since olives is more or less additive to the standard martini model, it makes sense to keep them separate.

With the new protein model of Luis', there are several reasons to keep martinize2 and olives as two separate calls, e.g. multistate olives would require you to martinize two proteins in 2 separate calls followed by one olives call etc..

I wont be able to maintain martinize2 going forward, but can support olives alone. From a software maintanece standpoint it is also not good to combine everything into a mega program.

Though, we could add olives to vermouth, so it would become a command line call like martinize2, such that you won't have to carry the python script around, but again one could just add olives to their .bashrc.

Best Kasper

@csbrasnett
Copy link
Contributor Author

@Lp0lp @paulocts @KasperBuskPedersen hopefully this satisfies everyone now!

@Lp0lp
Copy link
Contributor

Lp0lp commented Oct 3, 2024

I will have a look and get back to you!

@csbrasnett
Copy link
Contributor Author

@Lp0lp did you get a chance to have a look yet?

@Lp0lp
Copy link
Contributor

Lp0lp commented Oct 17, 2024

@csbrasnett not yet sorry. Had to tackle a couple of grants and currently on the road. Worst case I'll have it checked out by Monday. Sorry!
BTW I think it might be interesting to include the old protein tutorial on the deprecated tutorial sections. What do you think? Might have some benefit to someone trying to reproduce the old gomartini implementation.

Lp0lp added 2 commits October 21, 2024 14:34
Small changes to wording/formatting. Added old version of the tutorial as a legacy tutorial.
Added Martins collab tutorials.
Updated contributor list to reflect Chris and Martin.
@Lp0lp
Copy link
Contributor

Lp0lp commented Oct 21, 2024

Heyo!

@csbrasnett I am happy with the tutorial. I tweaked some wording and some formatting but nothing major. I've sent it to @paulocts so that he could have a look as well.

I added the old iteration of the tutorial to the legacy tutorials.

I also added to the tutorial list the two new collab tutorials @mnmelo and @martincalvelo cooked up. They end up working great in Quarto, since quarto automatically renders .ipynb as a webpage quite well. Only thing I changed was at the header to show authorship of the tutorial. @mnmelo and @martincalvelo I added the tutorials on the list as 4.1 and 4.2 since they are a natural continuation of tutorial 4 but I am open to listing them differently. What do you think?

I updated the tutorial author list to include @csbrasnett and @martincalvelo .

@csbrasnett
Copy link
Contributor Author

great, thanks!

All sounds good to me. To note, I'm currently looking at changing the behaviour of -scfix so it's used by default here. Once this is changed in martinize2 we can update here as well, so hold off on merging!

@mnmelo
Copy link

mnmelo commented Oct 21, 2024

Hey, this looks great. I'd add a caveat here, though:
Our tutorials were prepared for colab and if run blindly will download and install gromacs and dssp binaries as well as some pip dependencies. The most breaking stuff will probably fail unless run as root because it will attempt to unpack binaries onto system directories, but I imagine it can still be confusing...

Of course, non-Colab users should just provide their own dependencies and skip those installation cells. What's your take?

@Lp0lp
Copy link
Contributor

Lp0lp commented Oct 21, 2024 via email

@mnmelo
Copy link

mnmelo commented Oct 21, 2024

I like that kind of automation. It's safer. @martincalvelo, do you think you could try to incorporate that?

BTW, the code doesn't actually compile. It downloads a zip of already compiled GROMACS that I prepared and make available.

@martincalvelo
Copy link

Hi! I updated the two tutorials to detect whether they are running on Google Colab and, based on that, to install or not gromacs, dssp, and some python libraries. I also made some other small modifications to avoid incompatibilities when running locally.
The new files are on my GitHub:

Tutorial 4.1 Ceramide — VDAC1 binding in a membrane

Tutorial 4.2 Lipidomics study: Mag2 in a POPC and POPC-POPG 3:1 membrane

@csbrasnett
Copy link
Contributor Author

csbrasnett commented Dec 12, 2024

@Lp0lp, @danielpastor97 I think some users have had issues (eg this discussion)with the tutorial especially now that -govs-moltype has been removed from martinize. The changes are already there, please can this be merged asap?

@Lp0lp
Copy link
Contributor

Lp0lp commented Dec 12, 2024

I'll do the final tweaks today and then it should be good to go.

@danielpastor97
Copy link
Collaborator

Let me know if you need any assistance @Lp0lp . Looking good as far as I can tell!

@Lp0lp
Copy link
Contributor

Lp0lp commented Dec 12, 2024

All that was left to do was updating Martin and Manel's tutorials. Good to go on my end!

Copy link
Collaborator

@danielpastor97 danielpastor97 left a comment

Choose a reason for hiding this comment

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

@csbrasnett I have requested some small changes to ensure the Github Action we have implemented to deploy the website runs smoothly. Everything else looks great to me, thank you so much!

Once these are resolved I will proceed to merge it to main rigth away. Hopefully we'll see more updates like this for the other tutorials in the near future.

docs/tutorials/Legacy/martini3/ProteinsI/index.qmd Outdated Show resolved Hide resolved
docs/tutorials/Legacy/martini3/ProteinsI/index.qmd Outdated Show resolved Hide resolved
docs/tutorials/Legacy/martini3/ProteinsI/index.qmd Outdated Show resolved Hide resolved
@danielpastor97
Copy link
Collaborator

@csbrasnett Can you please check again the requested changes? I see you added two commits, but these don't resolve the points I mentioned.

  1. In the header of the announcement, the syntax is not correct. If we use the | symbol for an attribute, we can not write text right after it. In this case, that happens in the description: attribute, which prevents Quarto from compiling successfully. We can either remove the | symbol or use the symbol and add the text in the next line using a tab (as suggested in my review).
  2. In addition, the changes to the links were intended for the old ProteinsI tutorial that is now in legacy, not the new one. I will unresolve the requested changes so you can look at the comments. If you copy/paste the suggestions everything should be fine. Please make sure to undo the changes in the new tutorial added in the last commit, as these created additional broken links.

As soon as these changes are complete and the compilation of the website is running smoothly, I will proceed with the merge. Thanks again!🙂

@csbrasnett
Copy link
Contributor Author

Apologies for messing that up - hope all good now!

Copy link
Collaborator

@danielpastor97 danielpastor97 left a comment

Choose a reason for hiding this comment

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

This is perfect @csbrasnett. Merging to main now, changes should be live in a few minutes🚀🚀

@danielpastor97 danielpastor97 merged commit 63ba650 into Martini-Force-Field-Initiative:main Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants