Closed rwest closed 1 week ago
Cool, thanks for making these changes, Richard! I'm going to review this soon.
WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
I think this is a bit better.
I tried it on the examples/rmg/catalysis/ch4_o2/input.py
example.
The edge species got a bit out of hand....
But was it much better before? old-species.zip
I think the "before" pictures are probably clearer, for n>4 ?
Suggestions (comments welcome):
examples/rmg/catalysis/ch4_o2/input.py
example!examples/rmg/catalysis/methane_steam/input.py
working again.WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
I think this (skipping the new drawing method for n>4 surface sites) is a decent compromise?
WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
I see what you mean. But I can't currently think of a way to tweak the current algorithm, without a fairly specific "look for this exact case" change. I'll keep thinking though.
I found a few more issues. Think I've now fixed them. | Before this PR | before the most recent fix | latest version |
---|---|---|---|
The last one is still ugly, but at least none of them are misleading any more.
I updated the pictures at the top of the pull request with the latest version. I think we're good to go?
WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
This pull request is being automatically marked as stale because it has not received any interaction in the last 90 days. Please leave a comment if this is still a relevant pull request, otherwise it will automatically be closed in 30 days.
WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
I merged this in commit b30535b45e7033dd9f8ec1990ae8c5b2d4e1762d Unfortunately I messed up the interactions between github and the git command line and so github now thinks this pull request has zero commits and was closed, rather than containing many commits and being merged. Sorry. At least it's all there.
Motivation or Problem
Adsorbates with more than one surface site were drawn weirdly, with the second or third surface site often floating up in the air. Examples:
Description of Changes
With this change, it temporarily forms a bond between the surface sites, so the layout algorithm sees them as part of a ring, then it rotates the molecule so they are at the bottom, and then it removes the bond once the coordinates are determined. Update after many commits, the latest (2024-08-07) version looks like this:
You'll see that poly-dentate adsorbates with fused rings are still not perfect but 1) these are hard to do, even by hand 2) these are not worse than they used to be 3) our chemistry is so bad for these that for anyone with these in their model, pretty pictures are the least of their worries
...so I suggest fixing those is left for another day.
Testing
I made a unit test that draws a ton of polydentate adsorbates, then looked at them all (see above). This test set is mostly weirder than molecules that are likely to actually end up in a model core.
Keywords: multidentate bidentate polydentate