ReactionMechanismGenerator / RMG-website

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

Fix to various solvation display issues #225

Closed jonwzheng closed 3 years ago

jonwzheng commented 3 years ago

Issue - Solvent + Solute Molecular Displays

Currently there is a glitch s.t. in the entry view for solvents, the structure is never given. Also solvation data views only displays structure for solutes (such as here).

Proposed Solution Fixed the small error that was causing solvent molecules to not get rendered. Here is an example of the new entry page, compared to the old one. And here is a new solvation data page and another one when solvent_temp == 'None.

Current Issues

If the solvent SMILES string is too large (such as in the case of squalane) then the solvent structure fails to properly render.

For the vast majority of solvents this shouldn't be an issue, but still the issue exists for a few implemented solvents.

On molecule pages like this one, for solvents,, perhaps we can try to automatically link "Solvation" as one of the auto-linked properties.

Here is the table list view of all solvents, and of all solutes.

More commits added by Yunsie:

1. Fixing TypeError glitch related to missing solvent properties

2. Display more solvent, solute, and solvation group information

3. Update how solvation group database entries are displayed for special groups

4. Display detailed info for solvation group additivity

5. Miscellaneous changes

yunsiechung commented 3 years ago

@xiaoruiDong @jonwzheng I added some changes for solvation. I explained my changes on top and also explained why these changes can be tested. Do you mind reviewing this PR?

If everything looks okay, I will auto-squash the fixup commit and also add the python files created from making migrations.

jonwzheng commented 3 years ago

@yunsiechung, the changes look really good to me, awesome add-ons and fixups! A couple of small things before we approve. (1) We could merge parseThermoComment and parseSoluteDataComment. From what I can see, it looks like the changes between them are minor.
(2) When you do a solvation search it looks like the solvent info is no longer shown. Is this the intended display? (see old vs. new). (It looks like the solvent data is not recognized as an Entry in render_solvation_math, so its data is skipped.)

yunsiechung commented 3 years ago

@jonwzheng Thanks for the comments. (1) The way ThermoComment and SoluteDataComment are parsed are a bit different, especially for those that come from the library. So I would prefer to separate them into two different functions for now. (2) I actually forgot to display the solvent info for the solvation search. Instead of displaying the solvent data, I will create a link that directs the user to the solvent data page.

yunsiechung commented 3 years ago

@jonwzheng I added the change for your second comment.

I also added one more commit to edit some minor thing for solvation search. Previously, we only listed the solvents with both Abraham and Mintz parameters for solvation search. Now, we list all solvents for solvation search, and if the solvents only have some parameters, it will display only available results (e.g. if the solvent only has the Abraham parameters, it will only give the solvation free energy result and print 'N/A' for solvation enthalpy and entropy).

If everything looks good, I can also add the python files created from make migrations and push again.

yunsiechung commented 3 years ago

@jonwzheng I squashed the fixups. You can merge this PR now!

jonwzheng commented 3 years ago

Thanks! Before we merge, @xiaoruiDong do you know if we need to include the migrations files with this PR? I see we've done this in the past when models are updated, but our current workflow for updating the website seems to be designed not to include these files. Edit (7/19/21) - it looks like we do need to include these migrations files. There was a conflict when merging these files into the dev website, but it looks like the conflicting migration's changes are are already included in the migrations files in this PR. So the conflict was just from a mismatch in the names rather than content - which python manage.py makemigrations --merge fixed up automatically.