Skip to content

Replaced Chernov and Narayanaswamy libraries with "aromatic only" versions#132

Merged
zjburas merged 6 commits intoReactionMechanismGenerator:masterfrom
zjburas:master
Oct 24, 2016
Merged

Replaced Chernov and Narayanaswamy libraries with "aromatic only" versions#132
zjburas merged 6 commits intoReactionMechanismGenerator:masterfrom
zjburas:master

Conversation

@zjburas
Copy link
Copy Markdown
Contributor

@zjburas zjburas commented Oct 21, 2016

Deleted Chernov and Narayanaswamy kinetic and thermo libraries and replaced them with Chernov_aromatic_only and Narayanaswamy_aromatic_only libraries that have the following changes:

  1. As the name suggests, the kinetic libraries only include reactions that involve aromatics species. Non-aromatic species are still in the thermo libraries.
  2. Kekulized structures replaced with benzene bonds
  3. Erroneous CH2HCO thermo deleted from Chernov library.

Zachary Buras added 2 commits October 21, 2016 15:45
…placed them with

Chernov_aromatic_only and Narayanaswamy_aromatic_only libraries that have the following chages:
1. As the name suggests, the kinetic libraries only include reactions that involve aromatics species.
Non-aromatic species are still in the thermo libraries.
2. Kekulized structures replaced with benzene bonds
3. Erroneous CH2HCO thermo deleted from Chernov library.
@nyee
Copy link
Copy Markdown
Contributor

nyee commented Oct 21, 2016

@zjburas, At the top of the reactions.py and thermo_library.py file, you have to input the same name as the library name. My first guess looking at the Travis-failure is that you didn't change the name at the top to match the new names of Chernov_aromatic etc.

@zjburas
Copy link
Copy Markdown
Contributor Author

zjburas commented Oct 22, 2016

@nyee the names do match, but you reminded me to add a short and long description to the tops of each library. Still didn't pass the Tavis build. Any other suggestions?

@zjburas zjburas closed this Oct 22, 2016
@zjburas zjburas reopened this Oct 22, 2016
Combustion and Flame 157 (2010) 1879-1898
"""

"""
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You have extra quotes here. I think this is probably why Travis is failing.

@zjburas
Copy link
Copy Markdown
Contributor Author

zjburas commented Oct 24, 2016

Thanks for the suggestion @mliu49 , but the build still failed.

@rwest
Copy link
Copy Markdown
Member

rwest commented Oct 24, 2016

The errors seem to be Exception: Library Narayanaswamy not found in /home/travis/build/ReactionMechanismGenerator/RMG-database/input/thermo/libraries...Please check if your library is correctly placed
but the PR renamed the file input/thermo/libraries/Narayanaswamy.py.../libraries/Narayanaswamy_aromatic_only.py

@mliu49
Copy link
Copy Markdown
Contributor

mliu49 commented Oct 24, 2016

I was referring to the extra """ you had right before the helium entry, but you removed those when you removed the inerts, so that fixed a couple tests. The single quotes should have been fine.

Connie modified two tests to specifically load the Narayanaswamy library with this commit: ReactionMechanismGenerator/RMG-Py@37956ca. So it seems that the solution would either be to change the library being loaded in RMG-Py, or change the name of this library back to Narayanswamy.

The flux diagram test (test_avi) is also failing though, which I'm confused about, since it doesn't seem like you changed anything that would affect group additivity thermo estimates, which seems to be the part it's failing on.

@mliu49
Copy link
Copy Markdown
Contributor

mliu49 commented Oct 24, 2016

I just checked, and either solution will fix the build:

  1. Change nasaTest and wilhoitTest to use a different thermo library
  2. Rename Narayanaswamy_aromatic_only.py back to Narayanaswamy.py

Somehow, doing so also fixes the flux diagram test, for reasons which I don't understand.

@rwest
Copy link
Copy Markdown
Member

rwest commented Oct 24, 2016

Unless we have another version living alongside it I would vote we leave the name as Narayanaswamy. Makes history easier to track for one thing. Just make it crystal clear in the longDescription what the file does and does not contain

@zjburas
Copy link
Copy Markdown
Contributor Author

zjburas commented Oct 24, 2016

Thank you @mliu49 and @rwest for the quick debugging. I will go with fix 2 as you suggested, changing the name back to the original. I suppose the same should apply for Chernov, to be consistent.

@zjburas zjburas merged commit 62cecbe into ReactionMechanismGenerator:master Oct 24, 2016
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.

4 participants