feat: add first draft of nomeclature converters #10
Conversation
|
I still have to add the functions here to the modal app |
|
For the OPSIN calls http://cinfony.github.io might be worth a trial |
src/chemenv/tools/converters.py
Outdated
| return None | ||
|
|
||
| @backoff.on_exception(backoff.expo, (aiohttp.ClientError, asyncio.TimeoutError), max_time=10, logger=logger) | ||
| async def opsin(self) -> Optional[str]: |
There was a problem hiding this comment.
perhaps we could also run it locally (see the other comment). It should certainly also give more throughput.
src/chemenv/tools/converters.py
Outdated
| @backoff.on_exception(backoff.expo, (aiohttp.ClientError, asyncio.TimeoutError), max_time=10, logger=logger) | ||
| async def unknown(self) -> Optional[str]: | ||
| """ | ||
| Queries the Unknown API to convert chemical name to SMILES. | ||
|
|
||
| Returns: | ||
| str: SMILES notation if successful, None otherwise | ||
|
|
||
| Raises: | ||
| aiohttp.ClientError: On API connection errors | ||
| asyncio.TimeoutError: If request times out | ||
| """ | ||
| url = f"http://46.4.119.202:8082/?name=%7B{self.name}%7D&what=smiles" | ||
|
|
||
| async with aiohttp.ClientSession() as session: | ||
| try: | ||
| async with session.get(url, timeout=self.timeout) as response: | ||
| if response.status == 200: | ||
| message_content = await response.text() | ||
| message_text = message_content.split('Message:')[1].strip() | ||
| return message_text | ||
| logger.warning(f"Unknown API failed with status {response.status}") | ||
| return None | ||
| except Exception as e: | ||
| logger.error(f"Unknown API error: {e}") | ||
| return None |
There was a problem hiding this comment.
not sure if we should make this one public
There was a problem hiding this comment.
What do you think about the current solution? Maybe it can be refined so anyone can use it with a local database or a private endpoint
There was a problem hiding this comment.
nice that you also mocked some parts!
| @@ -4,6 +4,20 @@ | |||
| get_number_of_topologically_distinct_atoms as _get_topologically_distinct_atoms, | |||
| get_element_info as _get_element_info, | |||
| ) | |||
| from chemenv.tools.converters import ( | |||
| _converters_image, | |||
There was a problem hiding this comment.
what was the reason for making all of them semantically private (leading _)?
There was a problem hiding this comment.
The reason is that most of the functions of the app are called the same as the working functions, or they were at some point
src/chemenv/tools/converters.py
Outdated
| # Basic usage with IUPAC name | ||
| >>> converter = Name2Smiles("2-propanone") | ||
| >>> await converter.get_smiles() | ||
| 'CC(=O)C' |
There was a problem hiding this comment.
should this be indented 🤔
There was a problem hiding this comment.
Nice catch! Thanks!
src/chemenv/tools/converters.py
Outdated
| logger.error(f"Error encoding name: {e}") | ||
| raise ValueError(f"Invalid chemical name: {name}") | ||
|
|
||
| self.timeout = 10 # seconds |
There was a problem hiding this comment.
should we make this customizable via the constructor? Or is this a very robust setting that we will not often touch?
There was a problem hiding this comment.
In 6365350 they are customizable
| raise e | ||
|
|
||
| async def get_smiles(self) -> Optional[str]: | ||
| """Query all APIs in parallel until a valid SMILES is found. |
There was a problem hiding this comment.
hey - this is super cool! :)
src/chemenv/tools/converters.py
Outdated
| raise ValueError(f"Invalid SMILES: {smiles}") | ||
|
|
||
| self.smiles = smiles | ||
| self.timeout = 10 # seconds |
There was a problem hiding this comment.
similar question as above: should we move the timeout to the constructor?
| Query PubChem API to get IUPAC name from SMILES. | ||
|
|
||
| Returns: | ||
| Optional[str]: IUPAC name if found, None if the query failed. |
There was a problem hiding this comment.
nitpick: Is this the correct syntax for optional arguments in this docstring style?
There was a problem hiding this comment.
Unified into the Google PEP 484 type annotations in 6365350
There was a problem hiding this comment.
And thanks for the note!!
src/chemenv/tools/converters.py
Outdated
| Takes a SMILES and return the DeepSMILES encoding. | ||
|
|
||
| Args: | ||
| smiles: SMILES string |
There was a problem hiding this comment.
a micro-nitpick is that you could still add the type annotations to some of those docstrings
There was a problem hiding this comment.
in the medium term (a separate PR), it would probably be cool to have some RXN-SMILES converters. Those might not be useful for solving chembench but for your RXN project.
| mock_cactus = AsyncMock(return_value="ethanol") | ||
| mock_pubchem = AsyncMock(return_value=None) | ||
|
|
There was a problem hiding this comment.
very cool that you have such tests!
No description provided.