3
\$\begingroup\$

I made this python program to find the 64 most common elements of a list stored in a text file. Since this works using binomial names of trees I decided to use the number of mentions in PubMed Central as a metric of popularity. I am just a beginner and somewhat self-taught so I'd like to know if there are any ways of either optimizing this code or improving readability and whether it follows webscraping best practices. The output will be used to seed Facebook polls inside a botany group. Currently the input file has 81825 rows and can be found in this GitHub repository: https://github.com/ascyphermoonshot/larch-madness

import os
import random
import requests
import bs4
import re
from progress.bar import Bar
os.chdir(r"C:\Users\HP\Desktop\larch_madness") #or whatever your filepath is
trees=open('tree_list.txt',mode='r')
trees.seek(0)
treelist=trees.read().splitlines()
treelist=[tree.replace(" ","+").lower() for tree in treelist]
trees.close()
random.shuffle(treelist)
testlist=treelist
baseurl="https://www.ncbi.nlm.nih.gov/pmc/?term=%22{}%22"
finalists=[]
class Tree:
    def __init__(self,binomial,popularity):
        self.binomial=binomial.replace("+"," ").capitalize()
        self.popularity=popularity
    def __str__(self):
        return self.binomial
def popsearch(baseurl,tree):
    searchurl=baseurl.format(tree)
    try:
        sresult=requests.get(searchurl,timeout=10)
    except:
        try:
            sresult=requests.get(searchurl,timeout=10)
        except:
            try:
                sresult=requests.get(searchurl,timeout=10)
            except:
                #print("timeout")
                return None
    soup=bs4.BeautifulSoup(sresult.text,"lxml")
    def noresults(soup):
        stext=str(soup)
        return "No items found." in stext
    if not noresults(soup):
        try:
            resfound=str(soup.find('a', {'title' : 'Total Results'}).text)
            #print(resfound)
            num=re.search(r"(?<=\()\d+(?=\))",resfound)
            num=num.group(0)
            #print(num)
            return(int(num))
        except:
            #print ("not found")
            return None
    else:
        #print("no results")
        return None
def avoid_riots(finalists):
    favs=['Sequoiadendron giganteum','Rhizophora mangle','Ginkgo biloba','Hura crepitans','Taxus baccata','Sequoia sempervivens','Pinus longaeva','Ceiba petandra','Hippomane mancinella','Dracaena cinnabari','Juniperus virginiana','Dendrocnide moroides','Artocarpus heterophyllus','Litchi chinensis','Angiopteris evecta','Pyrus calleryana']
    diff=list(set(finalists[0:-15]) - set(favs))
    if len(diff)<0:
        for x in range(len(diff)):
            try:
                finalists.pop()
            except:
                pass
    finalists.extend(favs)
    finalists=set(finalists)
    return list(finalists)
if __name__ == '__main__':
    bar = Bar('Scraping', max=len(testlist),suffix='%(percent)d%%')
    for tree in testlist:
        #print(tree.replace("+"," ").capitalize())
        popularity=popsearch(baseurl,tree)
        if popularity!=None:
            finalists.append(Tree(tree,popularity))
            #print("found")
        bar.next()
    bar.finish()
    print("\n"*3)
    finalists.sort(key=lambda x: x.popularity, reverse=True)
    print(len(finalists))
    finalists=finalists[0:64]
    tfinalists=[x.binomial for x in finalists]
    tfinalists=avoid_riots(tfinalists)
    random.shuffle(tfinalists)
    for tree in tfinalists:
        print(tree)
    with open('final tree list.txt', 'w') as f:
        f.writelines("%s\n" % l for l in tfinalists)
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

There are several layers of non-ideal decisions in the design of this program, so let's peel them off:

Layer 1

Assuming that the data-gathering approach is good as-is, which it isn't - but we'll get to that later.

Application Design

I don't think it's a good idea to chdir() to a hard-coded directory, particularly one in Users. Either accept this as a parameter, or don't do it at all and assume that the current working directory is already correct.

if len(diff)<0 makes no sense because a collection length can never be negative. Assuming that this application works correctly, that entire block can be deleted because it will never evaluate to True.

Basic Python

Move global imperative code into functions.

Add PEP484 type hints.

Format your Python via PEP8 (get a linter or IDE to suggest how this is done).

Put file operations in a context manager, and don't explicitly close; guarantee the close call via with.

Don't seek(0); that's redundant for a newly-opened file.

Don't write a triple-nested exception block; just write a loop.

Use sets and set comprehensions more directly for your hilariously-titled avoid_riots logic.

Don't != None; write is not None because None is a singleton.

Delete your commented-out debug printing.

You don't need to provide a sortation lambda if you use a NamedTuple whose first element is the sortation key.

Split your favs literal to multiple lines. It can also be represented as a capitalised constant in the global namespace.

Library use

mode='r' is already the default so you don't need to do that.

Don't URLencode your data; let requests do that for you.

Don't bake your query into the URL; let requests do that for you.

Add a strainer so that BeautifulSoup can narrow its parsing scope.

Don't ignore HTTP errors; call raise_for_status.

Put the requests response object in context management.

Layer 2

You perform (and scrape) a full search and then throw away everything except for the result count. The PMC Advanced Search Builder offers a different interface that doesn't render any of the search results, only a summary of the search term, count and time. If you need to scrape (which you don't), use this instead. POST parameters are easily captured via reverse engineering in the browser.

Covering layers 1 and 2, your application could look like this. For simplicity I am limiting the number of search results and skipping the progress bar.

import random
from itertools import islice
from typing import Iterable, Iterator, NamedTuple

import bs4

from bs4.element import SoupStrainer
from requests import Session

HTTP_TRIES = 3
DEBUG_LIMIT = 5
TOP_POPULAR = 64
BASE_URL = 'https://www.ncbi.nlm.nih.gov/pmc'
STRAINER = SoupStrainer(name='table', id='HistoryView')

ENTREZ_QUERY = {
    'EntrezSystem2.PEntrez.PMC.Pmc_PageController.PreviousPageName': 'advanced',
    'EntrezSystem2.PEntrez.DbConnector.Cmd': 'Preview',
    'EntrezSystem2.PEntrez.DbConnector.Db': 'pmc',
    'p$l': 'EntrezSystem2',
}

FAVOURITES = {
    'Sequoiadendron giganteum', 'Rhizophora mangle', 'Ginkgo biloba', 'Hura crepitans', 'Taxus baccata',
    'Sequoia sempervivens', 'Pinus longaeva', 'Ceiba petandra', 'Hippomane mancinella', 'Dracaena cinnabari',
    'Juniperus virginiana', 'Dendrocnide moroides', 'Artocarpus heterophyllus', 'Litchi chinensis',
    'Angiopteris evecta', 'Pyrus calleryana',
}


class Tree(NamedTuple):
    popularity: int
    binomial: str

    def __str__(self) -> str:
        return self.binomial

    @classmethod
    def fetch_all(cls, tree_names: Iterable[str]) -> Iterator['Tree']:
        with Session() as session:
            for tree_name in tree_names:
                popularity = pop_search(session, tree_name)
                if popularity is not None:
                    yield cls(popularity, tree_name.capitalize())


def fetch_retry(session: Session, term: str) -> str:
    for _ in range(HTTP_TRIES):
        try:
            with session.post(
                url=BASE_URL,
                headers={'Accept': 'text/html'},
                timeout=10,
                data={
                    'EntrezSystem2.PEntrez.DbConnector.Term': term,
                    **ENTREZ_QUERY,
                },
            ) as sresult:
                sresult.raise_for_status()
                return sresult.text
        except IOError as e:
            last_ex = e

    raise last_ex


def parse(html: str, term: str) -> int:
    soup = bs4.BeautifulSoup(markup=html, features='lxml', parse_only=STRAINER)
    table: bs4.element.Tag = soup.contents[-1]
    heads = [th.text for th in table.find('thead').find_all('th')]
    rows = [
        {
            head: td.text
            for head, td in zip(heads, tr.find_all('td'))
        } for tr in table.find_all('tr', recursive=False)
    ]

    # The history interface is stateful. Assume that the first listed search term
    # is the one we care about. A more paranoid implementation could e.g. sort
    # antichronologically on the Search # column.
    record = next(
        row for row in rows
        if row['Query'] == f'Search {term}'
    )
    return int(record['Items found'])


def pop_search(session: Session, tree: str) -> int:
    term = f'"{tree.lower()}"'
    html = fetch_retry(session, term)
    return parse(html, term)


def load(filename: str = 'tree_list.txt') -> Iterator[str]:
    with open(filename) as f:
        for line in f:
            yield line.rstrip()


def avoid_riots(finalists: set[str]) -> set[str]:
    return finalists | FAVOURITES


def fetch_top(tree_names: Iterable[str]) -> list[Tree]:
    return sorted(
        islice(Tree.fetch_all(tree_names), DEBUG_LIMIT),
        reverse=True,
    )[:TOP_POPULAR]


def main() -> None:
    tree_names = list(load())
    random.shuffle(tree_names)
    trees = fetch_top(tree_names)

    names = {x.binomial for x in trees}
    augmented_names = list(avoid_riots(names))
    random.shuffle(augmented_names)

    print('\n'.join(augmented_names))
        

if __name__ == '__main__':
    main()
Ginkgo biloba
Dendrocnide moroides
Eucalyptus albida
Rhizophora mangle
Litchi chinensis
Angiopteris evecta
Hura crepitans
Orania oreophila
Ceiba petandra
Sequoia sempervivens
Sequoiadendron giganteum
Croton hancei
Taxus baccata
Utania nervosa
Hippomane mancinella
Juniperus virginiana
Artocarpus heterophyllus
Pinus longaeva
Barringtonia jebbiana
Dracaena cinnabari
Pyrus calleryana

Layer 3

Don't scrape! Some trivial googling reveals the E-Utilities API. This can much more narrowly target your use case, and is morally the "right thing to do" for a public service.

I demonstrate a functioning implementation that abides by the rate limitation constraints in the documentation; this can be sped up by getting an API key. I actually did get an API key, and added a logger to see what typical timing is. It turns out that basically all calls to the service take slightly longer than the minimum rate period, so as a consequence

  1. you might be able to get away with a simplified rate limitation strategy that simply issues requests with no delay, sleeping if you get a TOO_MANY_REQUESTS; and
  2. you might be able to add a threaded or asynchronous implementation that interleaves requests for higher performance while still staying within the limit.
import logging
import time
from contextlib import contextmanager
from http.client import TOO_MANY_REQUESTS
from itertools import islice
from random import shuffle
from typing import Iterable, Iterator, NamedTuple

from requests import Session

logger = logging.getLogger()

# See https://www.ncbi.nlm.nih.gov/books/NBK25497/#chapter2.Coming_in_December_2018_API_Key
API_KEY = None

UNAUTHENTICATED_RATE = 3
AUTHENTICATED_RATE = 10
RATE = AUTHENTICATED_RATE if API_KEY else UNAUTHENTICATED_RATE
HTTP_TRIES = 3
COOLOFF = 5
DEBUG_LIMIT = 10
TOP_POPULAR = 64

# See https://www.ncbi.nlm.nih.gov/books/NBK25499/#chapter4.ESearch
BASE_URL = 'https://eutils.ncbi.nlm.nih.gov/entrez/eutils/esearch.fcgi'

FAVOURITES = {
    'Sequoiadendron giganteum', 'Rhizophora mangle', 'Ginkgo biloba', 'Hura crepitans', 'Taxus baccata',
    'Sequoia sempervivens', 'Pinus longaeva', 'Ceiba petandra', 'Hippomane mancinella', 'Dracaena cinnabari',
    'Juniperus virginiana', 'Dendrocnide moroides', 'Artocarpus heterophyllus', 'Litchi chinensis',
    'Angiopteris evecta', 'Pyrus calleryana',
}


class Tree(NamedTuple):
    popularity: int
    binomial: str

    @classmethod
    def fetch_all(cls, session: Session, tree_names: Iterable[str]) -> Iterator['Tree']:
        for tree_name in tree_names:
            popularity = pop_search(session, tree_name)
            yield cls(popularity, tree_name.capitalize())


@contextmanager
def wait_rate() -> Iterator:
    # See for rate-limiting information:
    # https://www.ncbi.nlm.nih.gov/books/NBK25497/#chapter2.Frequency_Timing_and_Registrati
    # Unauthenticated rate limit is 3/second; authenticated is 10/s
    start = time.monotonic()
    yield
    dur = time.monotonic() - start
    wait_for = 1/RATE - dur + 0.01

    logger.debug('Would wait for %.3f s', wait_for)
    if wait_for > 0:
        time.sleep(wait_for)


def fetch_retry(session: Session, term: str) -> dict:
    params = {
        'db': 'pmc',
        'term': term,
        'usehistory': 'n',
        'rettype': 'count',
        'retmode': 'json',
    }
    if API_KEY:
        params['api_key'] = API_KEY

    for _ in range(HTTP_TRIES):
        with wait_rate():
            try:
                with session.get(
                    url=BASE_URL,
                    params=params,
                    headers={'Accept': 'application/json'},
                    timeout=10,
                ) as sresult:
                    if sresult.status_code == TOO_MANY_REQUESTS:
                        time.sleep(COOLOFF)
                    sresult.raise_for_status()
                    return sresult.json()
            except IOError as e:
                last_ex = e

    raise last_ex


def pop_search(session: Session, tree: str) -> int:
    term = f'"{tree.lower()}"'
    response = fetch_retry(session, term)
    return int(response['esearchresult']['count'])


def load(filename: str = 'tree_list.txt') -> Iterator[str]:
    with open(filename) as f:
        for line in f:
            yield line.rstrip()


def avoid_riots(finalists: set[str]) -> set[str]:
    return finalists | FAVOURITES


def fetch_top(tree_names: Iterable[str]) -> list[Tree]:
    with Session() as session:
        return sorted(
            islice(Tree.fetch_all(session, tree_names), DEBUG_LIMIT),
            reverse=True,
        )[:TOP_POPULAR]


def main() -> None:
    logging.basicConfig(level=logging.DEBUG)

    tree_names = list(load())
    shuffle(tree_names)
    trees = fetch_top(tree_names)

    names = {x.binomial for x in trees}
    augmented_names = list(avoid_riots(names))
    shuffle(augmented_names)

    print('\n'.join(augmented_names))


if __name__ == '__main__':
    main()

Output

With an API key defined. The negative delays mean that it never waits.

DEBUG:urllib3.connectionpool:Starting new HTTPS connection (1): eutils.ncbi.nlm.nih.gov:443
DEBUG:urllib3.connectionpool:https://eutils.ncbi.nlm.nih.gov:443 "GET /entrez/eutils/esearch.fcgi?db=pmc&term=%22endiandra+hypotephra%22&usehistory=n&rettype=count&retmode=json&api_key=******************** HTTP/1.1" 200 None
DEBUG:root:Would wait for -0.240 s
DEBUG:urllib3.connectionpool:https://eutils.ncbi.nlm.nih.gov:443 "GET /entrez/eutils/esearch.fcgi?db=pmc&term=%22byrsonima+formosa%22&usehistory=n&rettype=count&retmode=json&api_key=******************** HTTP/1.1" 200 None
DEBUG:root:Would wait for -0.072 s
DEBUG:urllib3.connectionpool:https://eutils.ncbi.nlm.nih.gov:443 "GET /entrez/eutils/esearch.fcgi?db=pmc&term=%22xylosma+raimondii%22&usehistory=n&rettype=count&retmode=json&api_key=******************** HTTP/1.1" 200 None
DEBUG:root:Would wait for -0.075 s
DEBUG:urllib3.connectionpool:https://eutils.ncbi.nlm.nih.gov:443 "GET /entrez/eutils/esearch.fcgi?db=pmc&term=%22cryptocarya+hypospodia%22&usehistory=n&rettype=count&retmode=json&api_key=******************** HTTP/1.1" 200 None
DEBUG:root:Would wait for -0.064 s
DEBUG:urllib3.connectionpool:https://eutils.ncbi.nlm.nih.gov:443 "GET /entrez/eutils/esearch.fcgi?db=pmc&term=%22erythroxylum+platyclados%22&usehistory=n&rettype=count&retmode=json&api_key=******************** HTTP/1.1" 200 None
DEBUG:root:Would wait for -0.100 s
DEBUG:urllib3.connectionpool:https://eutils.ncbi.nlm.nih.gov:443 "GET /entrez/eutils/esearch.fcgi?db=pmc&term=%22campylospermum+letouzeyi%22&usehistory=n&rettype=count&retmode=json&api_key=******************** HTTP/1.1" 200 None
DEBUG:root:Would wait for -0.047 s
DEBUG:urllib3.connectionpool:https://eutils.ncbi.nlm.nih.gov:443 "GET /entrez/eutils/esearch.fcgi?db=pmc&term=%22parkia+biglandulosa%22&usehistory=n&rettype=count&retmode=json&api_key=******************** HTTP/1.1" 200 None
DEBUG:root:Would wait for -0.073 s
DEBUG:urllib3.connectionpool:https://eutils.ncbi.nlm.nih.gov:443 "GET /entrez/eutils/esearch.fcgi?db=pmc&term=%22palicourea+schomburgkii%22&usehistory=n&rettype=count&retmode=json&api_key=******************** HTTP/1.1" 200 None
DEBUG:root:Would wait for -0.057 s
DEBUG:urllib3.connectionpool:https://eutils.ncbi.nlm.nih.gov:443 "GET /entrez/eutils/esearch.fcgi?db=pmc&term=%22dillenia+talaudensis%22&usehistory=n&rettype=count&retmode=json&api_key=******************** HTTP/1.1" 200 None
DEBUG:root:Would wait for -0.047 s
DEBUG:urllib3.connectionpool:https://eutils.ncbi.nlm.nih.gov:443 "GET /entrez/eutils/esearch.fcgi?db=pmc&term=%22stenocarpus+villosus%22&usehistory=n&rettype=count&retmode=json&api_key=******************** HTTP/1.1" 200 None
DEBUG:root:Would wait for -0.065 s
Hura crepitans
Sequoia sempervivens
Xylosma raimondii
Taxus baccata
Erythroxylum platyclados
Sequoiadendron giganteum
Rhizophora mangle
Ceiba petandra
Palicourea schomburgkii
Endiandra hypotephra
Stenocarpus villosus
Artocarpus heterophyllus
Campylospermum letouzeyi
Cryptocarya hypospodia
Byrsonima formosa
Ginkgo biloba
Dracaena cinnabari
Litchi chinensis
Angiopteris evecta
Parkia biglandulosa
Pyrus calleryana
Dendrocnide moroides
Pinus longaeva
Juniperus virginiana
Hippomane mancinella
Dillenia talaudensis
\$\endgroup\$
3
  • \$\begingroup\$ I'm intrigued about the second and third layer. Honestly I haven't used APIs yet and I should definitely read about it, although I'd like it if you could walk me through that implementation. My script is already running and should take about 20 hours to finish so even with the time limits this is a massive improvement. The avoid_riots is supposed to add the favourite trees if they are not among the 64 most popular and it's named like that because people in the group have very strong opinions about those plants. \$\endgroup\$ Jul 23 at 16:42
  • 1
    \$\begingroup\$ For the walk-through, chat.stackexchange.com/rooms/137992/… \$\endgroup\$
    – Reinderien
    Jul 23 at 18:44
  • 1
    \$\begingroup\$ BTW you caught a bug since it was supposed to be if len(diff)>0 not if len(diff)<0 which as you said makes no sense. That block should remove the last len(diff) elements to keep the length of the final list equal to 64. The conditional was only to catch errors in case all the favourites already appeared (because you can't loop 0 times and I think that'd crash the program if left unchecked). Thanks! \$\endgroup\$ Jul 24 at 3:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.