-
Notifications
You must be signed in to change notification settings - Fork 14
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
Added missing page titles #489
Conversation
Rebased on top of v2 branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @vedranmiletic - Thanks for the cleanup and apologies for the slow review.
In the wake of the xz backdoor I'm trying to do my due diligence in reviewing external PRs, and I got part of the way into reviewing the changes from this one before wondering - Does this need to change 700+ lines in 100+ files? The GitHub diff view isn't showing any characters changed in some highlighted lines in the diff (probably an issue with the diff view, but one I would like to understand before committing the changes), and very few of the changes appear to relate to the functioning of the website. Are all the changes to whitespace clearly needed here, or could this be cut down to a much smaller form that just adds the functional changes?
<iframe width="560" height="315" src="https://www.youtube-nocookie.com/embed/videoseries?si=ILxRak1DSQjGEJ3l&list=PLYW6oF6nr8Nv4FsU7jZgF_qnbid2TQisr" title="YouTube video player" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share" allowfullscreen></iframe> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What changed here?
<iframe width="560" height="315" src="https://www.youtube-nocookie.com/embed/videoseries?si=6BgwFhjraUXrFCzd&list=PLYW6oF6nr8NvEuaasDOcpZlQoy8NYsXDs" title="YouTube video player" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share" allowfullscreen></iframe> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
<iframe width="560" height="315" src="https://www.youtube-nocookie.com/embed/videoseries?si=XOLI03S0hrqZs1EF&list=PLYW6oF6nr8NuU1JeTHsMPaeYLYSjLhvSP" title="YouTube video player" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share" allowfullscreen></iframe> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
16.00 - 16.45 | [_Breakout session II:_ Force field assessment - protein-ligand free energies](https://docs.google.com/presentation/d/1VJup7h8lClRzF2Ei_rOF2Hvjx0aFXqYIVNGvhkxBBQ4/edit?usp=sharing) - [[discussion summary]](https://docs.google.com/presentation/d/1PCom76yRm12BXSRJZgLkGRJwt5FgIsVqlBKyoifxJ_A/edit?usp=sharing) | _Chair:_ John Chodera | ||
16.45 - 17.00 | _Breakout session II:_ Parameter definition, dataset selection, optimization strategies | _Chair:_ Lee-Ping Wang | ||
18.30 | **_Workshop dinner:_** [**_Rock Bottom Brewery_**](https://goo.gl/maps/F33PYtCxnw4U9oNQ8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
Currently, only 1D torsions are being computed because the price of 2D scans is 10-20x higher than for 1D scans. However, we plan to expand datasets both to include 2D torsions from the “Roche set” of molecules and more fragments from other molecules not currently included in the “Roche set”. The torsion drive data appears to [ForceBalance](https://github.com/leeping/forcebalance) as lists of data points consisting of coordinates, energies, and gradients; thus torsion drives of any dimension can be used when fitting force field parameters. | ||
|
||
The pipeline used to compute these torsions is [QCArchive](https://qcarchive.molssi.org/ ) → [TorsionDrive](https://github.com/lpwgroup/torsiondrive) → [geomeTRIC](https://github.com/leeping/geomeTRIC) → [Psi4](http://www.psicode.org/). The selected level of theory used to generate QM torsion profiles in this step was B3LYP-D3(BJ)/DZVP. The [SDF files](https://github.com/openforcefield/open-forcefield-data/blob/master/Torsion-Drives/Roche-Reference-Compounds/OpenFF_references.sdf) and [2D representations (pdf)](https://github.com/openforcefield/open-forcefield-data/blob/master/Torsion-Drives/Roche-Reference-Compounds/OpenFF_references.pdf) of molecules in “Roche set” are available on [GitHub](https://github.com/openforcefield/open-forcefield-data/tree/master/Torsion-Drives/Roche-Reference-Compounds), including all the [torsion profiles](https://github.com/openforcefield/open-forcefield-data/tree/master/Torsion-Drives/Roche-Reference-Compounds/Group1-820/b3lyp_631gs_local_ucd) calculated up to date. | ||
Currently, only 1D torsions are being computed because the price of 2D scans is 10-20x higher than for 1D scans. However, we plan to expand datasets both to include 2D torsions from the “Roche set” of molecules and more fragments from other molecules not currently included in the “Roche set”. The torsion drive data appears to [ForceBalance](https://github.com/leeping/forcebalance) as lists of data points consisting of coordinates, energies, and gradients; thus torsion drives of any dimension can be used when fitting force field parameters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
|
||
The pipeline used to compute these torsions is [QCArchive](https://qcarchive.molssi.org/ ) → [TorsionDrive](https://github.com/lpwgroup/torsiondrive) → [geomeTRIC](https://github.com/leeping/geomeTRIC) → [Psi4](http://www.psicode.org/). The selected level of theory used to generate QM torsion profiles in this step was B3LYP-D3(BJ)/DZVP. The [SDF files](https://github.com/openforcefield/open-forcefield-data/blob/master/Torsion-Drives/Roche-Reference-Compounds/OpenFF_references.sdf) and [2D representations (pdf)](https://github.com/openforcefield/open-forcefield-data/blob/master/Torsion-Drives/Roche-Reference-Compounds/OpenFF_references.pdf) of molecules in “Roche set” are available on [GitHub](https://github.com/openforcefield/open-forcefield-data/tree/master/Torsion-Drives/Roche-Reference-Compounds), including all the [torsion profiles](https://github.com/openforcefield/open-forcefield-data/tree/master/Torsion-Drives/Roche-Reference-Compounds/Group1-820/b3lyp_631gs_local_ucd) calculated up to date. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
Bond: 17.31004524230957 kJ / mol | ||
Angle: 195.2955780029297 kJ / mol | ||
Torsion: 13.520307540893555 kJ / mol | ||
Nonbonded: None | ||
vdW: 43.82065216824412 kJ / mol | ||
Electrostatics: -707.1108989715576 kJ / mol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these may be exact output characters from a program. Is there a pressing need to modify this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this removes hard tabs.
No, my apologies to you. I should have more clearly explained my motivations on the first submission.
That's completely understandable and I would worry the same way if I were you. In that regard, I believe my identity and track record is reasonably easy to check. After all, our community is quite small (compared to the global open-source developer community, at least) and publicly visible enough. The functional changes are in the first commit. The rest of the changes are clean-ups of Markdown files using markdownlint, which is a popular linter for Markdown syntax. I believe enforcing Markdown linting is a good practice and my former group's website uses it.
GitHub diff view indeed isn't perfect. I would suggest to check them locally commit-by-commit with As for merging, it could be either way, as you prefer. If you would be open to it, these "non-functional" changes can be the first step as I would be glad to contribute improvements to the deployment workflow with Hugo, i.e. also add linting action to the workflow pre-build. Then each run of the Hugo workflow on GitHub Actions would perform linting of the files and report errors. |
Perhaps you could open a separate PR that runs the linter(s) against the current The lint-only branch could also be merged first if we want to add a linter (I would, but don't feel strongly about it) |
Thanks for the quick response, @vedranmiletic. Could we scope this PR down to just the page titles and handle linting separately? The idea of adding a linting isn't bad, but 1) I feel weird accepting a diff larger than I can read (though I'm sure you're a cool person and the odds are low you're a fake account claiming to be said cool person), 2) not all the linting changes are clear improvements (eg changing the recorded output of a program) and it would take a long time to discuss them all, 3) adding a linting check to the build will complicate the lives of less-technical people who just want to update the website quickly and already find the GH workflow unfriendly. So, I'm happy to approve this PR if it's scaled back to just the first commit (and any other functional changes that I missed). |
Apologies for delay, done. |
Awesome. Thanks @vedranmiletic! |
This adds mising page titles on some pages and cleans up the overall Markdown file formatting. Feedback is welcome.