ReactionMechanismGenerator / RMG-website

A Django-powered website for Reaction Mechanism Generator (RMG)
Other
21 stars 29 forks source link

Add hyperlinks to group additivity thermo estimates #223

Closed jonwzheng closed 3 years ago

jonwzheng commented 3 years ago

Implements feature suggested in #214.

When group additivity thermo estimations are generated, typically a comment is added that informs what library and/or groups were used to generate that estimate. This comment is then displayed by the website. This implementation implements a new feature that automatically links to the relevant library or groups in the reported thermo comment whenever they are detected.

Sample implementation: (1) Species in original feature request (2) Example of handling missing groups (3) Unusual/many groups (4) Simple surface example (5) Edge case: No group additivity comment (6) Both library + group used to estimate gas phase thermo

This string depends on thermo comments being formatted partially in a specific way. It is somewhat flexible with detecting groups, but very structure-dependent for detecting libraries. Some other potential ways to implement this:

rwest commented 3 years ago

This looks like it'll be super helpful, and well done for getting it working.

I can't help wonder if the whole parseThermoComment(comment) method would be a lot simpler using regular expressions.

Plus then you'd be allowed to wear this shirt! (from the classic 2007 era xkcd comic)

...but perhaps "if it ain't broke, don't fix it" applies.

jonwzheng commented 3 years ago

This looks like it'll be super helpful, and well done for getting it working.

I can't help wonder if the whole parseThermoComment(comment) method would be a lot simpler using regular expressions.

Thanks for the feedback! I agree that a regex solution appears more elegant. But it becomes more complicated because there may be multiple parentheses groupings in a group name, and 2 substrings are desired from each matching string, so string parsing is easier to implement (imo).

xiaoruiDong commented 3 years ago

@jonwzheng The added comments really help, thanks! The commit history looks really weird though... e.g., there is a commit with changes related to openbabel3 (done in the last PR). Can you clean up those commits and let me know when you finish, and I will merge afterward. Thanks again!

jonwzheng commented 3 years ago

@jonwzheng The added comments really help, thanks! The commit history looks really weird though... e.g., there is a commit with changes related to openbabel3 (done in the last PR). Can you clean up those commits and let me know when you finish, and I will merge afterward. Thanks again!

OK, I squashed the commits into 1 commit. I think I had 2 problems here. (1) I was committing unnecessary changes, (2) I rebased incorrectly and incorporated those unnecessary changes again. So in the end I had to do some weird stuff with my fork's commit history. But I think it should be good to go now.

rwest commented 3 years ago

👏 Thanks!