ourresearch / oadoi

The backend code that powers Unpaywall. support@unpaywall.org
http://unpaywall.org
MIT License
311 stars 37 forks source link

Fix condition to merge author affiliations #134

Closed nolanm1122 closed 1 year ago

nolanm1122 commented 1 year ago

Current condition will be false (affiliations will not be merged) as long as at least one author has non-empty affiliations. I assume condition should be the inverse (affiliations should merge if at least one author has empty affiliations).

caseydm commented 1 year ago

I'd like to consider one scenario that could happen after this change. Say we have one out of two affiliations from crossref, so that means we merge what is in parseland. But then parseland has no affiliations. Will parseland overwrite the 1 affiliation we had, leaving both null? If it would, maybe we should check that there is at least one valid affiliation in parseland before merging the affiliations. I think in most instances we either get all or none affiliations, so if we at least have one we should go with what is in parseland. Please check!

caseydm commented 1 year ago

Or maybe the solution is to do something like this, where we fallback to what existed if it is there:

if cls._should_merge_affiliations(record, pub): crossref_author['affiliation'] = pl_authors[best_match_idx].get('affiliation', []) or record.authors[best_match_idx].get('affiliation', [])

nolanm1122 commented 1 year ago

@caseydm Yeah I think this is a scenario that could happen. I went ahead and made the change you suggested.

caseydm commented 1 year ago

Perfect thanks! I approved it and will go ahead and deploy it.