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

Add parser for glass transition temperature #13

Merged
merged 1 commit into from
Feb 2, 2017
Merged

Conversation

rtchoua
Copy link
Contributor

@rtchoua rtchoua commented Jan 25, 2017

…ditions to extract melting point using Tm.

@rtchoua
Copy link
Contributor Author

rtchoua commented Jan 25, 2017

I got this when I run pytest:
Ran 348 test cases in 29.89s (12.40s CPU), 1 errors
28 modules OK (1 failed)
failures: /home/roselyne/ChemDataExtractor/tests/test_nlp_tokenize [1/120]
I don't think I have modified that file. I added some test for tg and tm and I believe they passed. Let me know if I can do anything else. Thanks!

@rtchoua rtchoua closed this Jan 25, 2017
@rtchoua rtchoua reopened this Jan 25, 2017
Copy link
Owner

@mcs07 mcs07 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@@ -460,6 +469,9 @@ def merge_contextual(self, other):
for item in self[k]:
# print('item: %s' % item)
for other_item in other.get(k, []):
#RBT Problem with doi: ma301230y. Had to add following check
if (isinstance(other_item,unicode) == True):
continue
Copy link
Owner

Choose a reason for hiding this comment

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

Seems this problem occurs when it is attempted to merge the 'contextual' information from a compound record into another one, but the 'contextual' record also happens to contain a name/label/role. This can happen when merging in information from table footnotes or caption - the idea is to merge e.g. a solvent into the property in the table cell that references it. But sometimes there is an actual compound named in the footnote. This fix seems to work well - just ignoring the string-based properties - which are name/label/role. However, unicode is python 2 only - this needs to be six.text_type for python 2 and 3 compatibility. In future, this whole merge_contextual method needs refactoring to be more agnostic of the model schema.

c.quantum_yields = [QuantumYield(**context)]
c.fluorescence_lifetimes = [FluorescenceLifetime(**context)]
c.electrochemical_potentials = [ElectrochemicalPotential(**context)]
c.uvvis_spectra = [UvvisSpectrum(**context)]
Copy link
Owner

Choose a reason for hiding this comment

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

Good catch on the missing ** - I forgot to update this when I changed the model objects to accept keyword arguments rather than a single dictionary. The ** unpacks the context dict to be the equivalent of kwargs like solvent='acetonitrile etc.

@mcs07 mcs07 merged commit e5dab19 into mcs07:master Feb 2, 2017
@mcs07 mcs07 changed the title Adding code to extract (glass transition temperature) Tg and minor ad… Add parser for glass transition temperature Feb 2, 2017
@rtchoua
Copy link
Contributor Author

rtchoua commented Feb 7, 2017 via email

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

Successfully merging this pull request may close these issues.

2 participants