Add cgnf embeddings #145
Add cgnf embeddings #145
Conversation
WalkthroughThe changes introduce a new representation scheme to the project, outlining detailed steps for adding necessary files, updating documentation, and modifying configurations. Additionally, they correct entity names and apply version constraints to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (3)
docs/reference.md (2)
Line range hint
55-55: Markdown formatting issue: Trailing spacesPlease remove the trailing spaces at the end of the line to adhere to Markdown best practices.
- <details> + <details>Tools
LanguageTool
[uncategorized] ~3-~3: This verb does not appear to agree with the subject. Consider using a different form. (AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
Context: ... The data contained in this repository are a collection of various elemental repre...
Line range hint
64-64: Markdown formatting issue: Lists should be surrounded by blank linesEnsure that lists are surrounded by blank lines to adhere to Markdown best practices and improve readability.
- <details> + + <details> +Also applies to: 144-144
Tools
LanguageTool
[uncategorized] ~3-~3: This verb does not appear to agree with the subject. Consider using a different form. (AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
Context: ... The data contained in this repository are a collection of various elemental repre...src/elementembeddings/core.py (1)
Line range hint
252-252: Recommendation to addstacklevelin warningIt is a good practice to specify the
stacklevelwhen issuing warnings to help users understand where exactly in their code the issue originates.- warnings.warn( + warnings.warn("Embedding is already standardised. Returning None and not changing the embedding.", stacklevel=2)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- README.md (1 hunks)
- docs/reference.md (5 hunks)
- src/elementembeddings/core.py (1 hunks)
- src/elementembeddings/utils/config.py (2 hunks)
Additional context used
LanguageTool
docs/reference.md
[uncategorized] ~3-~3: This verb does not appear to agree with the subject. Consider using a different form. (AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
Context: ... The data contained in this repository are a collection of various elemental repre...
[uncategorized] ~36-~36: When ‘Materials-Agnostic’ is used as a modifier, it is usually spelled with a hyphen. (SPECIFIC_HYPHEN)
Context: ...wing paper describes the details of the Materials Agnostic Platform for Informatics and Exploratio...README.md
[uncategorized] ~24-~24: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ... these involve the use of deep learning techniques where the representation of the element...
[uncategorized] ~34-~34: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...mbeddings's main feature, the Embedding class is accessible by importing the class. ...
[uncategorized] ~140-~140: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...ing through theembeddingandstatsarguments respectively. ```python from elementem...
Markdownlint
docs/reference.md
55-55: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
64-64: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
144-144: null (MD032, blanks-around-lists)
Lists should be surrounded by blank linesREADME.md
172-172: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
Ruff
src/elementembeddings/core.py
252-252: No explicit
stacklevelkeyword argument found (B028)
Additional comments not posted (5)
src/elementembeddings/utils/config.py (2)
16-16: Addition of new embedding entry is correctThe addition of the
"cgnf": "cgnf.json"entry toDEFAULT_ELEMENT_EMBEDDINGScorrectly follows the existing pattern for embedding paths and aligns with the PR's objective to introduce cgnf embeddings.
150-159: Correct addition of citation details for 'cgnf'The citation details for the newly added 'cgnf' embedding have been correctly added to the
CITATIONSdictionary. The format adheres to the existing citation style and provides comprehensive reference information.docs/reference.md (1)
27-33: New section for 'cgnf' correctly addedThe addition of a new section for 'cgnf' in the vector representations is well-documented. The references and data source links are correctly formatted and provide useful information for users wanting to understand or utilize the 'cgnf' embedding.
README.md (1)
169-176: Clear instructions for adding an embedding schemeThe steps provided for adding a new embedding scheme are clear and comprehensive. This documentation will assist users in contributing new embedding schemes to the project effectively.
Tools
Markdownlint
172-172: null (MD032, blanks-around-lists)
Lists should be surrounded by blank linessrc/elementembeddings/core.py (1)
63-63: Proper addition of 'Cgnf' to embedding optionsThe addition of 'Cgnf' to the list of available embeddings in the
load_datamethod is correctly implemented. This change allows users to easily load the 'Cgnf' data using the established interface.
| ### Add an embedding scheme | ||
|
|
||
| The steps required to add a new representation scheme are: | ||
| 1. Add data file to [data/element_representations](src/elementembeddings/data/element_representations). |
There was a problem hiding this comment.
Markdown formatting issue: Lists should be surrounded by blank lines
Ensure that lists are surrounded by blank lines to adhere to Markdown best practices and improve readability.
- 1. Add data file to [data/element_representations](src/elementembeddings/data/element_representations).
+
+ 1. Add data file to [data/element_representations](src/elementembeddings/data/element_representations).
+ Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 1. Add data file to [data/element_representations](src/elementembeddings/data/element_representations). | |
| 1. Add data file to [data/element_representations](src/elementembeddings/data/element_representations). | |
Tools
Markdownlint
172-172: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/elementembeddings/utils/config.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/elementembeddings/utils/config.py
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #145 +/- ##
=======================================
Coverage 92.55% 92.55%
=======================================
Files 12 12
Lines 1047 1047
=======================================
Hits 969 969
Misses 78 78 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- requirements.txt (1 hunks)
- setup.py (1 hunks)
Files skipped from review due to trivial changes (2)
- requirements.txt
- setup.py
Pull Request Template
Description
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Self-test (no new functions)
Reviewers
@AntObi
Checklist:
Summary by CodeRabbit
Documentation
README.md.reference.md.Bug Fixes
core.py.Chores
requirements.txtandsetup.pyto restrictnumpyversion to below 2.