Code review#4
Open
Essmaw wants to merge 27 commits into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request removes several legacy dataset artifacts from data/, including two example Zenodo JSON records and multiple plain-text gazetteer lists for entity types (molecules, force fields, software names/versions, temperatures, and simulation times).
Changes:
- Delete two sample annotated record files:
data/zenodo_34415.jsonanddata/zenodo_1009027.json. - Delete multiple legacy gazetteer
.txtlists:TEMP.txt,STIME.txt,SOFTVERS.txt,SOFTNAME.txt,MOL.txt,FFM.txt. - Leave the currently referenced inputs (e.g.,
data/entities.tsv,data/*_ground_truth.json,data/llm_prompt.txt) untouched.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| data/zenodo_34415.json | Removed sample annotated Zenodo record JSON from the repository. |
| data/zenodo_1009027.json | Removed sample annotated Zenodo record JSON from the repository. |
| data/TEMP.txt | Removed legacy temperature gazetteer list. |
| data/STIME.txt | Removed legacy simulation-time gazetteer list. |
| data/SOFTVERS.txt | Removed legacy software-version gazetteer list. |
| data/SOFTNAME.txt | Removed legacy software-name gazetteer list. |
| data/MOL.txt | Removed legacy molecule gazetteer list. |
| data/FFM.txt | Removed legacy force-field gazetteer list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…rmalization processes.
…I and improve file handling
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 39 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (6)
src/mdverse_entity_norm/scripts/normalize_stemp.py:101
temperature_unitcan beNone(e.g., when the input has no explicit unit), which makesstr(temperature_value) + temperature_unitraiseTypeError. This will stop the whole script during TSV generation.
src/mdverse_entity_norm/scripts/normalize_stemp.py:119- The CLI entrypoint docstring says it performs a visualization, but the visualization code has been removed. This is misleading for users reading
--helpor docs generated from docstrings.
src/mdverse_entity_norm/scripts/evaluate_llm_models.py:402 - The TSV header still uses
normalisation_times_sec/normalisation_cost, but the rows now writeinference_time_by_entity/inference_cost_by_entity_USD. This mismatch makes the output hard to interpret and contradicts the key names being emitted.
src/mdverse_entity_norm/scripts/evaluate_llm_models.py:414 --groundtruth-pathhas no default but is not marked required. If omitted, Click will passNoneand the script will fail later when reading the file; this should be validated at the CLI layer.
src/mdverse_entity_norm/scripts/evaluate_llm_models.py:425--model-evaluation-pathis required for output but is not marked required and does not ensure the parent directory exists. If omitted,save_evaluation_results_in_tsvwill be called withNoneand fail downstream.
src/mdverse_entity_norm/scripts/evaluate_llm_models.py:430--prompt-pathis not marked required and does not validate existence / file-ness. Omitting it will passNoneinto the prompt loader and fail with a less clear error than Click can provide.
Comment on lines
134
to
138
| @click.option( | ||
| "--entities-file", | ||
| "--entities-path", | ||
| type=click.Path(exists=True, dir_okay=False, path_type=Path), | ||
| default=Path("../data/ground_entities.tsv"), | ||
| help="Path to the TSV file containing the entities.", | ||
| ) |
Comment on lines
139
to
143
| @click.option( | ||
| "--output-file", | ||
| "--normalization-results-path", | ||
| type=click.Path(dir_okay=False, path_type=Path), | ||
| default=Path("../results/norm_simu_times/normalized_stime_results.tsv"), | ||
| help="Path to the output TSV file for normalized results.", | ||
| ) |
Comment on lines
+144
to
+148
| @click.option( | ||
| "--prompt-path", | ||
| type=click.Path(file_okay=True, path_type=Path), | ||
| help="Path to the llm prompt file", | ||
| ) |
Comment on lines
+149
to
+153
| @click.option( | ||
| "--model-name", | ||
| type=str, | ||
| help="Name of the LLM model to use for normalization.", | ||
| ) |
Comment on lines
+54
to
+58
| @click.option( | ||
| "--log-file", | ||
| type=click.Path(exists=True, dir_okay=False, path_type=Path), | ||
| help="Path to the TSV file containing the entities.", | ||
| ) |
| # mdverse_entity_norm | ||
|
|
||
| This project implements the normalization pipeline for molecular dynamics (MD) simulation metadata entities. Normalization standardizes entity mentions by mapping them to controlled vocabularies or reference databases (e.g., [ChEBI](https://www.ebi.ac.uk/chebi/), [PubChem](https://pubchem.ncbi.nlm.nih.gov/), [KEGG](https://www.genome.jp/kegg/)), ensuring consistency and interoperability across datasets. | ||
| Normalisation is currently supported for four entity types: molecule names (MOL),simulation times (STIME) and temperatures (STEMP), software names (SOFTNAME) and force fields and models (FFM). |
| | raw_temperature | normalised_temperature | normalised_unit | normalized_result | | ||
| | --------------- | ---------------------- | --------------- | ----------------- | | ||
| | 315 | 315 | K | 315 K | | ||
| | 20°C | 293,15 | K | 293,15 K | |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.