Skip to content

WIP: Scival Publication Lookup #377

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

montypanday
Copy link

Implemented the new Scival submodule with 1 function Publication lookup

Have added tests to test the properties

Uses the utils and superclasses

Given the list of Scival API's here - https://dev.elsevier.com/api_docs.html
I would appreciate some feedback on the naming of classes so I can improve the PR.

I plan to build all of the functions available, Proposing this PR for early feedback.

Happy to amend this PR based on feedback.

Your help on this request is much appreciated.

Linked to #203 #253

Copy link
Contributor

@Michael-E-Rose Michael-E-Rose left a comment

Choose a reason for hiding this comment

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

This is extremely solid work, thank you very much! It took me a while to review your elaborated PR, but only because I wanted to give it the attention it deserves! I have just a few requests for more documentation.

@Michael-E-Rose
Copy link
Contributor

Awesome work @montypanday ! I'm sure many people will find this utterly useful!

I've got a question: None of the SciVal APIs has views, right?

@montypanday montypanday changed the title Scival submodule WIP: Scival submodule Feb 11, 2025
@montypanday montypanday changed the title WIP: Scival submodule WIP: Scival Publication Lookup Feb 12, 2025
@montypanday
Copy link
Author

Hi @Michael-E-Rose,

Thank you for your feedback. I have made the requested changes.

  1. Docstrings added as requested.
  2. Tests split into tests for each attribute or group of attributes
  3. Yes, you are correct that Views are not in any of the Scival API's. Hence, I propose that no empty addition to the VIEWS configuration should be needed. The only effect is on the creation of cache directories. If an API does not support views, we do not need to create sub directory for each view.
  4. Have sorted the constants in alphabetical order of class names.

With this change adding a Lookup class for the following API's should be fairly simple.
We might need a new superclass called Metrics for Metric retrieval. I am working on it.

SciVal Author Lookup
SciVal Country Lookup
SciVal Country Group Lookup
SciVal Institution Lookup
SciVal Institution Group Lookup
SciVal Publication Lookup
SciVal Scopus Source Lookup
SciVal Subject Area
SciVal Topic Lookup
SciVal Topic Cluster Lookup
SciVal World Lookup

My apologies for the delay in response.

Kind regards

Comment on lines +118 to +123
views = VIEWS.get(api, [])
if views:
for view in views:
(base_dir / view).mkdir(parents=True, exist_ok=True)
else:
base_dir.mkdir(parents=True, exist_ok=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can simplify this. The test is not necessary because the loop doesn't trigger for an empty list. But in this case we need the base_path to be triggered. We achieve that with

views = VIEWS.get(api, [""])  # empty string
for view in views:
    cache_folder = path / view
    cache_folder.mkdir(parent=True, exist_ok=True)

Comment on lines +12 to +58
def test_publication_id():
assert pub1.id == 85036568406


def test_publication_doi():
assert pub1.doi == "10.1002/anie.201709271"


def test_publication_type():
assert pub1.type == "Article"


def test_publication_year():
assert pub1.publication_year == 2017


def test_publication_source_title():
assert pub1.source_title == 'Angewandte Chemie - International Edition'


def test_publication_citation_count():
assert pub1.citation_count > 0


def test_publication_authors_count():
assert len(pub1.authors) >= 7


def test_publication_first_author():
assert pub1.authors[0].id == 7404861905
assert pub1.authors[0].name == "Lin, T.-E."


def test_publication_institutions_count():
assert len(pub1.institutions) >= 3


def test_publication_first_institution():
assert pub1.institutions[0].id == 217002
assert pub1.institutions[0].name == "Chang Gung University"
assert pub1.institutions[0].country == "Taiwan"
assert pub1.institutions[0].country_code == "TWN"


def test_publication_sdgs():
assert len(pub1.sdgs) >= 1
assert pub1.sdgs[0] == 'SDG 3: Good Health and Well-being'
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind sorting them alphabetically, please?

Copy link
Contributor

@Michael-E-Rose Michael-E-Rose left a comment

Choose a reason for hiding this comment

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

Great! Almost there. I am just asking for two changes.

Once you feel you are done, please remove the WIP from the PR title. Otherwise I cannot merge.

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.

3 participants