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

Added missing page titles #489

Merged
merged 1 commit into from
Jun 18, 2024
Merged

Conversation

vedranmiletic
Copy link
Contributor

@vedranmiletic vedranmiletic commented Apr 25, 2024

This adds mising page titles on some pages and cleans up the overall Markdown file formatting. Feedback is welcome.

@vedranmiletic
Copy link
Contributor Author

Rebased on top of v2 branch.

Copy link
Member

@j-wags j-wags left a 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&amp;list=PLYW6oF6nr8Nv4FsU7jZgF_qnbid2TQisr" title="YouTube video player" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share" allowfullscreen></iframe>
Copy link
Member

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&amp;list=PLYW6oF6nr8NvEuaasDOcpZlQoy8NYsXDs" title="YouTube video player" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share" allowfullscreen></iframe>
Copy link
Member

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&amp;list=PLYW6oF6nr8NuU1JeTHsMPaeYLYSjLhvSP" title="YouTube video player" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share" allowfullscreen></iframe>
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Comment on lines 59 to 61
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)
Copy link
Member

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.
Copy link
Member

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.
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Comment on lines 262 to 267
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
Copy link
Member

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?

Copy link
Contributor Author

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.

@vedranmiletic
Copy link
Contributor Author

Hi @vedranmiletic - Thanks for the cleanup and apologies for the slow review.

No, my apologies to you. I should have more clearly explained my motivations on the first submission.

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?

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.

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?

GitHub diff view indeed isn't perfect. I would suggest to check them locally commit-by-commit with git diff. Note that many of the fixes are automatically performed by markdownlint-cli2 tool.

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.

@mattwthompson
Copy link
Member

Perhaps you could open a separate PR that runs the linter(s) against the current v2 branch, then comparing this branch to that one would highlight the substantive changes? I use markdownlint myself and like it, but all of these changes make it hard to suss out what's automated and what's not.

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)

@j-wags
Copy link
Member

j-wags commented Jun 7, 2024

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).

@vedranmiletic
Copy link
Contributor Author

Apologies for delay, done.

@mattwthompson mattwthompson merged commit 4418631 into openforcefield:v2 Jun 18, 2024
2 checks passed
@j-wags
Copy link
Member

j-wags commented Jun 18, 2024

Awesome. Thanks @vedranmiletic!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants