Documentation Changes #57
Conversation
|
Warning Rate limit exceeded@justin13601 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 15 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis update mainly focuses on documentation enhancements and minor code adjustments for better clarity and usability. It includes reorganizing sections, refining descriptions, and adding new details in various markdown and Jupyter notebook files. Additionally, there's a change in the LaTeX configuration in the Sphinx Changes
Sequence Diagram(s)None 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/source/index.md (2)
Line range hint
57-57: Consider simplifying the sentence to enhance clarity.- This approach not only simplifies the process but also ensures high-quality, reproducible results for ... + This approach simplifies the process and ensures high-quality, reproducible results for ...
Line range hint
57-57: Reduce the number of exclamation marks to maintain a professional tone in the documentation.- ... datasets in reasonable raw formulations! + ... datasets in reasonable raw formulations.docs/source/conf.py (1)
Line range hint
58-58: The import statement forosis not at the top of the file, which is against PEP 8 guidelines.+ import os - import osMove the
import osstatement to the top of the file to comply with PEP 8.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- docs/source/conf.py (1 hunks)
- docs/source/index.md (1 hunks)
- docs/source/profiling.md (1 hunks)
Files skipped from review due to trivial changes (1)
- docs/source/profiling.md
Additional context used
LanguageTool
docs/source/index.md
[style] ~57-~57: Opting for a less wordy alternative here can improve the clarity of your writing. (NOT_ONLY_ALSO)
Context: ...ithin the ACES ecosystem. This approach not only simplifies the process but also ensures high-quality, reproducible results for ...
[style] ~57-~57: Using many exclamation marks might seem excessive (in this case: 8 exclamation marks for a text that’s 2716 characters long) (EN_EXCESSIVE_EXCLAMATION)
Context: ... datasets in reasonable raw formulations!
Ruff
docs/source/conf.py
58-58: Module level import not at top of file (E402)
Additional comments not posted (2)
docs/source/index.md (1)
21-21: Renaming "Profiling" to "Computational Profiling" enhances the specificity of the section title.docs/source/conf.py (1)
324-324: Switching toxelatexas the LaTeX engine is appropriate for better font and Unicode support in the documentation.
docs/source/conf.py
Outdated
| # "preamble": "\n".join( | ||
| # [ | ||
| # r"\usepackage{svg}", | ||
| # r"\DeclareUnicodeCharacter{2501}{-}", | ||
| # r"\DeclareUnicodeCharacter{2503}{|}", | ||
| # r"\DeclareUnicodeCharacter{2500}{-}", | ||
| # r"\DeclareUnicodeCharacter{2550}{-}", | ||
| # r"\DeclareUnicodeCharacter{2517}{+}", | ||
| # r"\DeclareUnicodeCharacter{2518}{+}", | ||
| # r"\DeclareUnicodeCharacter{2534}{+}", | ||
| # r"\DeclareUnicodeCharacter{250C}{+}", | ||
| # r"\DeclareUnicodeCharacter{252C}{+}", | ||
| # r"\DeclareUnicodeCharacter{2510}{+}", | ||
| # r"\DeclareUnicodeCharacter{2502}{|}", | ||
| # r"\DeclareUnicodeCharacter{2506}{|}", | ||
| # r"\DeclareUnicodeCharacter{2561}{|}", | ||
| # r"\DeclareUnicodeCharacter{256A}{|}", | ||
| # r"\DeclareUnicodeCharacter{2523}{|}", | ||
| # r"\DeclareUnicodeCharacter{03BC}{\ensuremath{\mu}}", | ||
| # r"\DeclareUnicodeCharacter{255E}{|}", | ||
| # r"\DeclareUnicodeCharacter{255F}{+}", | ||
| # r"\DeclareUnicodeCharacter{254E}{|}", | ||
| # r"\DeclareUnicodeCharacter{257C}{-}", | ||
| # r"\DeclareUnicodeCharacter{257E}{-}", | ||
| # r"\DeclareUnicodeCharacter{2559}{+}", | ||
| # ] | ||
| # ), |
There was a problem hiding this comment.
The large block of LaTeX preamble settings has been commented out. If this is intentional for simplification, consider removing it entirely to clean up the configuration file.
- # "preamble": "\n".join(
- # [
- # r"\usepackage{svg}",
- # ...
- # r"\DeclareUnicodeCharacter{2559}{+}",
- # ]
- # ),
+ # Preamble settings removed for simplification.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.
| # "preamble": "\n".join( | |
| # [ | |
| # r"\usepackage{svg}", | |
| # r"\DeclareUnicodeCharacter{2501}{-}", | |
| # r"\DeclareUnicodeCharacter{2503}{|}", | |
| # r"\DeclareUnicodeCharacter{2500}{-}", | |
| # r"\DeclareUnicodeCharacter{2550}{-}", | |
| # r"\DeclareUnicodeCharacter{2517}{+}", | |
| # r"\DeclareUnicodeCharacter{2518}{+}", | |
| # r"\DeclareUnicodeCharacter{2534}{+}", | |
| # r"\DeclareUnicodeCharacter{250C}{+}", | |
| # r"\DeclareUnicodeCharacter{252C}{+}", | |
| # r"\DeclareUnicodeCharacter{2510}{+}", | |
| # r"\DeclareUnicodeCharacter{2502}{|}", | |
| # r"\DeclareUnicodeCharacter{2506}{|}", | |
| # r"\DeclareUnicodeCharacter{2561}{|}", | |
| # r"\DeclareUnicodeCharacter{256A}{|}", | |
| # r"\DeclareUnicodeCharacter{2523}{|}", | |
| # r"\DeclareUnicodeCharacter{03BC}{\ensuremath{\mu}}", | |
| # r"\DeclareUnicodeCharacter{255E}{|}", | |
| # r"\DeclareUnicodeCharacter{255F}{+}", | |
| # r"\DeclareUnicodeCharacter{254E}{|}", | |
| # r"\DeclareUnicodeCharacter{257C}{-}", | |
| # r"\DeclareUnicodeCharacter{257E}{-}", | |
| # r"\DeclareUnicodeCharacter{2559}{+}", | |
| # ] | |
| # ), | |
| # Preamble settings removed for simplification. |
Codecov ReportAll modified and coverable lines are covered by tests ✅ |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
docs/source/conf.py (1)
Line range hint
58-58: Move thesysmodule import to the top of the file to adhere to Python best practices.+ import sys ... - import sys
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- README.md (1 hunks)
- docs/source/conf.py (1 hunks)
Additional context used
Ruff
docs/source/conf.py
58-58: Module level import not at top of file (E402)
LanguageTool
README.md
[uncategorized] ~243-~243: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...eld for a "derived" predicate: -expr: Must be a string with the 'and()' or 'o...
[uncategorized] ~245-~245: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...e any other predicate manually defined (ie., setting a constraint on its occurrence...
[uncategorized] ~251-~251: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ... and can be directly used:_ANY_EVENT: specifies any event in the data (ie., e...
[uncategorized] ~251-~251: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...VENT: specifies any event in the data (ie., effectively set to1` for every singl...
[uncategorized] ~253-~253: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...r predicates dataframe)_RECORD_START: specifies the beginning of a patient's ...
[uncategorized] ~253-~253: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...es the beginning of a patient's record (ie., effectively set to1in the first ch...
[uncategorized] ~255-~255: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...w for everysubject_id)_RECORD_END: specifies the end of a patient's record...
[uncategorized] ~255-~255: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...pecifies the end of a patient's record (ie., effectively set to1in the last chr...
[style] ~271-~271: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’. (HYPHENATED_LY_ADVERB_ADJECTIVE)
Context: ...Windows Windows can be of two types, a temporally-bounded window or an event-bounded window. Belo...
[style] ~271-~271: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’. (HYPHENATED_LY_ADVERB_ADJECTIVE)
Context: ...event-bounded window. Below is a sample temporally-bounded window configuration: ``` trigger: adm...
[uncategorized] ~285-~285: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...e, the windowinputbegins atNULL(ie., the first event or the start of the ti...
[uncategorized] ~285-~285: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: .... The window is inclusive on both ends (ie., both the first event and the event at ...
[uncategorized] ~285-~285: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE_2)
Context: ...traint of 5 events of any kind is placed so any valid window would include sufficie...
[uncategorized] ~287-~287: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...tive time period expressed as a string (ie.,+ 2 days,- 365 days,+ 12h, `- ...
[uncategorized] ~287-~287: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...directional arrow and a predicate name (ie.,-> predicate_1or<- predicate_1)....
[uncategorized] ~291-~291: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...ed by usingNoneor leaving it empty (ie.,(5, None),(8,),(None, 32), `(,...
[uncategorized] ~291-~291: Commas should not be placed before a closing parenthesis. Either move the comma outside of the parentheses, or remove it altogether. (COMMA_CLOSING_PARENTHESIS)
Context: ... or leaving it empty (ie.,(5, None),(8,),(None, 32),(,10)).labelis a...
[uncategorized] ~295-~295: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ... at which the prediction would be made (ie., set toendin your window containing...
Markdownlint
README.md
69-69: Punctuation: ':' (MD026, no-trailing-punctuation)
Trailing punctuation in heading
81-81: Punctuation: ':' (MD026, no-trailing-punctuation)
Trailing punctuation in heading
190-190: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
273-273: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
Additional comments not posted (2)
docs/source/conf.py (1)
324-324: Change oflatex_engineto "xelatex" is approved.README.md (1)
309-310: Placeholder for additional commentary in the "Future Roadmap" section is noted. Please specify what type of commentary or content is expected here to provide clearer guidance to contributors or readers.
| @@ -321,7 +321,7 @@ def ensure_pandoc_installed(_): | |||
|
|
|||
|
|
|||
| # -- Options for LaTeX output | |||
|
|
|||
| # latex_engine = "xelatex" | |||
There was a problem hiding this comment.
Consider removing the commented-out preamble settings if they are no longer needed to clean up the configuration file.
- "preamble": "\n".join(
- [
- r"\usepackage{svg}",
- ...
- r"\DeclareUnicodeCharacter{2559}{+}",
- ]
- ),Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
README.md (3)
Line range hint
243-243: Correct the abbreviation "i.e." to "i.e.," as per the standard grammatical rules.- ie., + i.e.,Also applies to: 245-245, 251-251, 253-253, 255-255, 285-285, 287-287, 291-291, 295-295
Line range hint
271-271: The hyphen in "temporally-bounded" and "event-bounded" is unnecessary when the adverb ends in 'ly'.- temporally-bounded + temporally bounded - event-bounded + event bounded
Line range hint
190-190: Specify the language for fenced code blocks to enable proper syntax highlighting.- ``` + ```yamlAlso applies to: 273-273
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- README.md (1 hunks)
- docs/source/license.md (1 hunks)
- docs/source/profiling.md (1 hunks)
Files skipped from review due to trivial changes (1)
- docs/source/license.md
Files skipped from review as they are similar to previous changes (1)
- docs/source/profiling.md
Additional context used
LanguageTool
README.md
[uncategorized] ~22-~22: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...yses can also become more accessible to clinicians as it enables the extraction of tailore...
[uncategorized] ~243-~243: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...eld for a "derived" predicate: -expr: Must be a string with the 'and()' or 'o...
[uncategorized] ~245-~245: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...e any other predicate manually defined (ie., setting a constraint on its occurrence...
[uncategorized] ~251-~251: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ... and can be directly used:_ANY_EVENT: specifies any event in the data (ie., e...
[uncategorized] ~251-~251: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...VENT: specifies any event in the data (ie., effectively set to1` for every singl...
[uncategorized] ~253-~253: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...r predicates dataframe)_RECORD_START: specifies the beginning of a patient's ...
[uncategorized] ~253-~253: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...es the beginning of a patient's record (ie., effectively set to1in the first ch...
[uncategorized] ~255-~255: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...w for everysubject_id)_RECORD_END: specifies the end of a patient's record...
[uncategorized] ~255-~255: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...pecifies the end of a patient's record (ie., effectively set to1in the last chr...
[style] ~271-~271: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’. (HYPHENATED_LY_ADVERB_ADJECTIVE)
Context: ...Windows Windows can be of two types, a temporally-bounded window or an event-bounded window. Belo...
[style] ~271-~271: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’. (HYPHENATED_LY_ADVERB_ADJECTIVE)
Context: ...event-bounded window. Below is a sample temporally-bounded window configuration: ``` trigger: adm...
[uncategorized] ~285-~285: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...e, the windowinputbegins atNULL(ie., the first event or the start of the ti...
[uncategorized] ~285-~285: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: .... The window is inclusive on both ends (ie., both the first event and the event at ...
[uncategorized] ~285-~285: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE_2)
Context: ...traint of 5 events of any kind is placed so any valid window would include sufficie...
[uncategorized] ~287-~287: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...tive time period expressed as a string (ie.,+ 2 days,- 365 days,+ 12h, `- ...
[uncategorized] ~287-~287: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...directional arrow and a predicate name (ie.,-> predicate_1or<- predicate_1)....
[uncategorized] ~291-~291: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...ed by usingNoneor leaving it empty (ie.,(5, None),(8,),(None, 32), `(,...
[uncategorized] ~291-~291: Commas should not be placed before a closing parenthesis. Either move the comma outside of the parentheses, or remove it altogether. (COMMA_CLOSING_PARENTHESIS)
Context: ... or leaving it empty (ie.,(5, None),(8,),(None, 32),(,10)).labelis a...
[uncategorized] ~295-~295: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ... at which the prediction would be made (ie., set toendin your window containing...
Markdownlint
README.md
69-69: Punctuation: ':' (MD026, no-trailing-punctuation)
Trailing punctuation in heading
81-81: Punctuation: ':' (MD026, no-trailing-punctuation)
Trailing punctuation in heading
190-190: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
273-273: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
README.md
Outdated
| ### Complementary Tools | ||
|
|
||
| % Using ACES in Concert with Other tools | ||
|
|
||
| ACES is a component of the MEDS ecosystem; to learn more about other tools with which you can use ACES, please see the following: | ||
| \\begin\{itemize} | ||
| \\item To extract data in the MEDS format, LINK | ||
| \\item To produce automated tabular (XGBoost) baseline methods over ACES defined task dataframes for MEDS datasets, see LINK | ||
| \\item ... | ||
| \\end\{itemize} | ||
|
|
There was a problem hiding this comment.
Consider adding actual links where 'LINK' is mentioned to provide complete information.
README.md
Outdated
| ### Alternative Tools | ||
|
|
||
| TODO | ||
|
|
||
| ## Future Roadmap | ||
|
|
||
| TODO add more commentary |
There was a problem hiding this comment.
Please provide more details or remove the placeholder if it's not intended to be filled immediately.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
docs/source/index.md (2)
Line range hint
56-56: Consider simplifying the sentence to enhance clarity.- This approach not only simplifies the process but also ensures high-quality, reproducible results for ... + This approach simplifies the process and ensures high-quality, reproducible results for ...
Line range hint
56-56: Reduce the number of exclamation marks to maintain a professional tone in documentation.- ... datasets in reasonable raw formulations! + ... datasets in reasonable raw formulations.README.md (1)
Line range hint
243-243: Address minor grammatical issues to enhance document professionalism.- ie., setting a constraint on its occurrence + i.e., setting a constraint on its occurrence - ie., effectively set to `1` for every single row in your predicates dataframe + i.e., effectively set to `1` for every single row in your predicates dataframe - ie., effectively set to `1` in the first chronological row for every `subject_id` + i.e., effectively set to `1` in the first chronological row for every `subject_id` - ie., effectively set to `1` in the last chronological row for every `subject_id` + i.e., effectively set to `1` in the last chronological row for every `subject_id` - ie., the first event or the start of the time series record + i.e., the first event or the start of the time series record - ie., `-> predicate_1` or `<- predicate_1` + i.e., `-> predicate_1` or `<- predicate_1` - ie., `(5, None)`, `(8,)`, `(None, 32)`, `(,10)` + i.e., `(5, None)`, `(8,)`, `(None, 32)`, `(,10)` - ie., set to `end` in your window containing input data + i.e., set to `end` in your window containing input data - ie., XGBoost over ACES-defined tasks + i.e., XGBoost over ACES-defined tasks - a variety of schemas + various schemasAlso applies to: 251-251, 253-253, 255-255, 285-285, 287-287, 291-291, 295-295, 308-308, 317-317
Tools
LanguageTool
[uncategorized] ~308-~308: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...ate automated tabular baseline methods (ie., XGBoost over ACES-defined tasks). - [M...
[style] ~317-~317: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it. (A_VARIETY_OF)
Context: ...task definitions, and can be applied to a variety of schemas, making it a versatile tool sui...
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- README.md (1 hunks)
- docs/source/index.md (1 hunks)
- docs/source/notebooks/predicates.ipynb (3 hunks)
- docs/source/profiling.md (1 hunks)
- docs/source/technical.md (1 hunks)
Files skipped from review due to trivial changes (1)
- docs/source/technical.md
Files skipped from review as they are similar to previous changes (1)
- docs/source/profiling.md
Additional context used
LanguageTool
docs/source/index.md
[style] ~56-~56: Opting for a less wordy alternative here can improve the clarity of your writing. (NOT_ONLY_ALSO)
Context: ...ithin the ACES ecosystem. This approach not only simplifies the process but also ensures high-quality, reproducible results for ...
[style] ~56-~56: Using many exclamation marks might seem excessive (in this case: 8 exclamation marks for a text that’s 2715 characters long) (EN_EXCESSIVE_EXCLAMATION)
Context: ... datasets in reasonable raw formulations!README.md
[uncategorized] ~243-~243: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...eld for a "derived" predicate: -expr: Must be a string with the 'and()' or 'o...
[uncategorized] ~245-~245: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...e any other predicate manually defined (ie., setting a constraint on its occurrence...
[uncategorized] ~251-~251: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ... and can be directly used:_ANY_EVENT: specifies any event in the data (ie., e...
[uncategorized] ~251-~251: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...VENT: specifies any event in the data (ie., effectively set to1` for every singl...
[uncategorized] ~253-~253: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...r predicates dataframe)_RECORD_START: specifies the beginning of a patient's ...
[uncategorized] ~253-~253: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...es the beginning of a patient's record (ie., effectively set to1in the first ch...
[uncategorized] ~255-~255: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...w for everysubject_id)_RECORD_END: specifies the end of a patient's record...
[uncategorized] ~255-~255: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...pecifies the end of a patient's record (ie., effectively set to1in the last chr...
[style] ~271-~271: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’. (HYPHENATED_LY_ADVERB_ADJECTIVE)
Context: ...Windows Windows can be of two types, a temporally-bounded window or an event-bounded window. Belo...
[style] ~271-~271: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’. (HYPHENATED_LY_ADVERB_ADJECTIVE)
Context: ...event-bounded window. Below is a sample temporally-bounded window configuration: ``` trigger: adm...
[uncategorized] ~285-~285: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...e, the windowinputbegins atNULL(ie., the first event or the start of the ti...
[uncategorized] ~285-~285: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: .... The window is inclusive on both ends (ie., both the first event and the event at ...
[uncategorized] ~285-~285: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE_2)
Context: ...traint of 5 events of any kind is placed so any valid window would include sufficie...
[uncategorized] ~287-~287: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...tive time period expressed as a string (ie.,+ 2 days,- 365 days,+ 12h, `- ...
[uncategorized] ~287-~287: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...directional arrow and a predicate name (ie.,-> predicate_1or<- predicate_1)....
[uncategorized] ~291-~291: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...ed by usingNoneor leaving it empty (ie.,(5, None),(8,),(None, 32), `(,...
[uncategorized] ~291-~291: Commas should not be placed before a closing parenthesis. Either move the comma outside of the parentheses, or remove it altogether. (COMMA_CLOSING_PARENTHESIS)
Context: ... or leaving it empty (ie.,(5, None),(8,),(None, 32),(,10)).labelis a...
[uncategorized] ~295-~295: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ... at which the prediction would be made (ie., set toendin your window containing...
[uncategorized] ~308-~308: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...ate automated tabular baseline methods (ie., XGBoost over ACES-defined tasks). - [M...
[style] ~317-~317: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it. (A_VARIETY_OF)
Context: ...task definitions, and can be applied to a variety of schemas, making it a versatile tool sui...
Markdownlint
README.md
69-69: Punctuation: ':' (MD026, no-trailing-punctuation)
Trailing punctuation in heading
81-81: Punctuation: ':' (MD026, no-trailing-punctuation)
Trailing punctuation in heading
190-190: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
273-273: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
Additional comments not posted (4)
docs/source/index.md (1)
19-20: Renaming sections for clarity enhances document navigation.docs/source/notebooks/predicates.ipynb (2)
29-41: Updating empty values to "null" improves data consistency and clarity.
98-98: Emphasizing manual creation of complex predicates prepares users for future updates.README.md (1)
303-317: Adding a section on complementary tools and comparisons provides valuable context for users.Tools
LanguageTool
[uncategorized] ~308-~308: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...ate automated tabular baseline methods (ie., XGBoost over ACES-defined tasks). - [M...
[style] ~317-~317: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it. (A_VARIETY_OF)
Context: ...task definitions, and can be applied to a variety of schemas, making it a versatile tool sui...
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (2)
docs/source/configuration.md (2)
30-30: Clarify punctuation usage in lists.Consider using consistent punctuation in lists to improve readability. For example, ensure that all items in a list either end with a period or none do, depending on whether they are complete sentences.
Also applies to: 33-33, 41-41, 45-45, 47-47, 53-53, 59-59, 86-86, 91-91, 92-92, 133-133, 140-140
Tools
LanguageTool
[uncategorized] ~30-~30: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...consists of three parts: -predicates, stored as a dictionary from string pred...
Line range hint
49-49: Simplify language to enhance clarity.- with these options being decided on the basis of `value_min_inclusive` + based on `value_min_inclusive` - with these options being decided on the basis of `value_max_inclusive` + based on `value_max_inclusive`This change reduces wordiness and improves the flow of the text.
Also applies to: 55-55
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- docs/source/configuration.md (2 hunks)
- docs/source/index.md (2 hunks)
- docs/source/notebooks/tutorial.ipynb (2 hunks)
- docs/source/terminology.md (1 hunks)
Files skipped from review due to trivial changes (1)
- docs/source/notebooks/tutorial.ipynb
Additional context used
LanguageTool
docs/source/index.md
[style] ~56-~56: Opting for a less wordy alternative here can improve the clarity of your writing. (NOT_ONLY_ALSO)
Context: ...ithin the ACES ecosystem. This approach not only simplifies the process but also ensures high-quality, reproducible results for ...
[style] ~56-~56: Using many exclamation marks might seem excessive (in this case: 8 exclamation marks for a text that’s 2719 characters long) (EN_EXCESSIVE_EXCLAMATION)
Context: ... datasets in reasonable raw formulations!docs/source/configuration.md
[uncategorized] ~30-~30: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...consists of three parts: -predicates, stored as a dictionary from string pred...
[uncategorized] ~33-~33: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...t build on other predicates. -trigger, stored as a string toEventConfig- `...
[uncategorized] ~41-~41: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...cateConfig####PlainPredicateConfig`: Configuration of Predicates that can be...
[uncategorized] ~45-~45: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ... of the following four fields: -code: The string value for the categorical co...
[uncategorized] ~47-~47: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...s code in the observation. -value_min: If specified, an observation will only ...
[style] ~49-~49: ‘on the basis of’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_ON_THE_BASIS_OF)
Context: ..._min(with these options being decided on the basis ofvalue_min_inclusive, wherevalue_m...
[uncategorized] ~53-~53: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ... equal to will be used). -value_max: If specified, an observation will only ...
[style] ~55-~55: ‘on the basis of’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_ON_THE_BASIS_OF)
Context: ..._max(with these options being decided on the basis ofvalue_max_inclusive, wherevalue_m...
[uncategorized] ~59-~59: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ... will be used). -value_min_inclusive: Seevalue_min-value_max_inclusive...
[uncategorized] ~86-~86: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ... column. ####DerivedPredicateConfig: Configuration of Predicates that Depend...
[uncategorized] ~91-~91: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ... -and(pred_1_name, pred_2_name, ...): Asserts that all of the specified predi...
[style] ~91-~91: Consider removing “of” to be more concise (ALL_OF_THE)
Context: ..._name, pred_2_name, ...): Asserts that all of the specified predicates must be true. -o...
[uncategorized] ~92-~92: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...e. -or(pred_1_name, pred_2_name, ...): Asserts that any of the specified predi...
[uncategorized] ~133-~133: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...REFERENCING = $REFERENCED + $TIME_DELTA,$REFERENCING = $REFERENCED - $TIME_DEL...
[uncategorized] ~140-~140: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...REFERENCING = $REFERENCED -> $PREDICATE,$REFERENCING = $REFERENCED <- $PREDICA...docs/source/terminology.md
[grammar] ~3-~3: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive. (TO_NON_BASE)
Context: ...ch details events that have happened to subjects. Each row in the dataframe will have a ...
[style] ~8-~8: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it. (A_VARIETY_OF)
Context: ... of columns which describe the event in a variety of ways. These columns can either have a b...
[style] ~43-~43: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it. (A_VARIETY_OF)
Context: ...in this particular case could represent a variety of other events in the medical record, suc...
[grammar] ~88-~88: Did you mean “exists”? (THERE_VBP_A_NN)
Context: ...n a "valid" form for a patient if there exist a set of time points such that, within ...
[uncategorized] ~141-~141: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...s strictly increasing in our algorithm (ie., the start of a "window" will always be...
[style] ~202-~202: ‘Prior to’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_PRIOR_TO)
Context: ...#### Identify Prospective Root Anchors Prior to summarizing the rest of the task tree, ...
[style] ~238-~238: ‘In the event that’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_IN_THE_EVENT_THAT)
Context: ...ng the recursive step. #####offsetIn the event that the subtree root timestamp is not the s...
[grammar] ~241-~241: The modal verb ‘will’ requires the verb’s base form. (MD_BASEFORM)
Context: ... If the two are not the same, they will guaranteed to be separated by a constantoffset...
[uncategorized] ~283-~283: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ... realization of this subtree's anchor event and contains aTruevalue otherwise. ...
[uncategorized] ~331-~331: The preposition ‘as’ seems more likely in this position. (AI_HYDRA_LEO_REPLACE_TO_AS)
Context: ...his timestamp can be manually specified to any start or end timestamp of any desir...
[uncategorized] ~333-~333: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...t which point a prediction can be made (ie., at the end of theinputwindows). #...
Markdownlint
docs/source/terminology.md
123-123: Expected: h3; Actual: h4 (MD001, heading-increment)
Heading levels should only increment by one level at a time
97-97: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
Additional comments not posted (3)
docs/source/index.md (1)
18-20: Rename sections to enhance clarity and specificity.The renaming of "Profiling" to "Computational Profiling" and "Configuration Language" to "Technical Details" improves specificity and clarity, aligning with the content described in these sections.
docs/source/configuration.md (1)
1-1: Ensure the introduction is clear and concise.The introduction clearly outlines the purpose and scope of the configuration language, setting the stage for the detailed specifications that follow.
docs/source/terminology.md (1)
1-1: Clarify the introduction to the algorithm and design section.The introduction effectively sets the context for the detailed explanation of the algorithm used in the project, providing a clear overview of the data structure and its components.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (9)
docs/source/terminology.md (9)
1-1: Update the document title to maintain consistency with other documentation standards.Consider using "Terminology and Design" instead of "Terminology & Design" for consistency with formal writing standards in documentation.
Line range hint
8-8: Consider revising the phrase "a variety of" to enhance clarity.The phrase "a variety of" can be wordy. Consider replacing it with "various" to make the writing clearer and more concise.
Also applies to: 43-43
Tools
LanguageTool
[grammar] ~3-~3: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive. (TO_NON_BASE)
Context: ...ch details events that have happened to subjects. Each row in the dataframe will have a ...
Line range hint
88-88: Correct grammatical error regarding the existence of time points.- if there exist a set of time points + if there exists a set of time pointsTools
LanguageTool
[grammar] ~3-~3: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive. (TO_NON_BASE)
Context: ...ch details events that have happened to subjects. Each row in the dataframe will have a ...
Line range hint
141-141: Correct the abbreviation for "i.e." to include two periods.- algorithm (ie., the start of a "window" + algorithm (i.e., the start of a "window"- made (ie., at the end of the `input` windows). + made (i.e., at the end of the `input` windows).Also applies to: 333-333
Tools
LanguageTool
[grammar] ~3-~3: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive. (TO_NON_BASE)
Context: ...ch details events that have happened to subjects. Each row in the dataframe will have a ...
Line range hint
202-202: Simplify the phrase "Prior to" for conciseness.Consider replacing "Prior to" with "Before" to reduce wordiness and enhance readability.
Tools
LanguageTool
[grammar] ~3-~3: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive. (TO_NON_BASE)
Context: ...ch details events that have happened to subjects. Each row in the dataframe will have a ...
Line range hint
238-238: Simplify the phrase "In the event that" for conciseness.Consider replacing "In the event that" with "If" to reduce wordiness and enhance readability.
Tools
LanguageTool
[grammar] ~3-~3: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive. (TO_NON_BASE)
Context: ...ch details events that have happened to subjects. Each row in the dataframe will have a ...
Line range hint
241-241: Correct the grammatical error in the use of the modal verb 'will'.- they will guaranteed to be separated by a constant `offset` + they will be guaranteed to be separated by a constant `offset`Tools
LanguageTool
[grammar] ~3-~3: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive. (TO_NON_BASE)
Context: ...ch details events that have happened to subjects. Each row in the dataframe will have a ...
Line range hint
97-97: Add a language specifier to the fenced code block.Please specify a language for the fenced code block to enhance syntax highlighting and readability. For YAML, you can use:
- ```yaml + ```yaml trigger: admission gap: start: trigger end: trigger + 48h excludes: - discharge - death - covid_dx target: start: gap.end end: discharge | death label: death excludes: - covid_dx input: end: trigger + 24hTools
LanguageTool
[grammar] ~3-~3: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive. (TO_NON_BASE)
Context: ...ch details events that have happened to subjects. Each row in the dataframe will have a ...
Line range hint
123-123: Correct the heading level for consistency.Please adjust the heading level from h4 to h3 to maintain a consistent structure throughout the document.
- #### Event + ### EventTools
LanguageTool
[grammar] ~3-~3: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive. (TO_NON_BASE)
Context: ...ch details events that have happened to subjects. Each row in the dataframe will have a ...
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- docs/source/profiling.md (1 hunks)
- docs/source/technical.md (1 hunks)
- docs/source/terminology.md (1 hunks)
- src/aces/configs/init.py (1 hunks)
Files skipped from review due to trivial changes (1)
- src/aces/configs/init.py
Files skipped from review as they are similar to previous changes (2)
- docs/source/profiling.md
- docs/source/technical.md
Additional context used
LanguageTool
docs/source/terminology.md
[grammar] ~3-~3: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive. (TO_NON_BASE)
Context: ...ch details events that have happened to subjects. Each row in the dataframe will have a ...
[style] ~8-~8: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it. (A_VARIETY_OF)
Context: ... of columns which describe the event in a variety of ways. These columns can either have a b...
[style] ~43-~43: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it. (A_VARIETY_OF)
Context: ...in this particular case could represent a variety of other events in the medical record, suc...
[grammar] ~88-~88: Did you mean “exists”? (THERE_VBP_A_NN)
Context: ...n a "valid" form for a patient if there exist a set of time points such that, within ...
[uncategorized] ~141-~141: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...s strictly increasing in our algorithm (ie., the start of a "window" will always be...
[style] ~202-~202: ‘Prior to’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_PRIOR_TO)
Context: ...#### Identify Prospective Root Anchors Prior to summarizing the rest of the task tree, ...
[style] ~238-~238: ‘In the event that’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_IN_THE_EVENT_THAT)
Context: ...ng the recursive step. #####offsetIn the event that the subtree root timestamp is not the s...
[grammar] ~241-~241: The modal verb ‘will’ requires the verb’s base form. (MD_BASEFORM)
Context: ... If the two are not the same, they will guaranteed to be separated by a constantoffset...
[uncategorized] ~333-~333: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...t which point a prediction can be made (ie., at the end of theinputwindows). #...
Markdownlint
docs/source/terminology.md
123-123: Expected: h3; Actual: h4 (MD001, heading-increment)
Heading levels should only increment by one level at a time
97-97: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
| # Algorithm & Design | ||
|
|
||
| ## Introduction | ||
| ## Terminology & Design | ||
|
|
||
| We will assume that we are given a dataframe `df` which details events that have happened to subjects. Each |
There was a problem hiding this comment.
Address grammatical issue in the introductory paragraph.
- which details events that have happened to subjects.
+ detailing events that have occurred to subjects.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.
| We will assume that we are given a dataframe `df` which details events that have happened to subjects. Each | |
| We will assume that we are given a dataframe `df` detailing events that have occurred to subjects. Each |
Tools
LanguageTool
[grammar] ~3-~3: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive. (TO_NON_BASE)
Context: ...ch details events that have happened to subjects. Each row in the dataframe will have a ...
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)
- docs/source/profiling.md (1 hunks)
- src/aces/configs/init.py (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- docs/source/profiling.md
- src/aces/configs/init.py
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (7)
docs/source/terminology.md (7)
Line range hint
8-8: Consider using less wordy expressions to enhance readability.- These columns can either have a binary value (1/0) representing whether certain properties are True/False for each row's event, or a count (integer) for the number of times that certain properties hold within each row's event. + These columns may have a binary value (1/0) indicating True/False properties for each row's event, or a count (integer) for the number of times those properties occur within each row's event. - Events that don't satisfy any predicates in this particular case could represent a variety of other events in the medical record, such as a lab test, a procedure, or a non-COVID diagnosis, just to name a few. + Events without predicates might represent other medical record events, like lab tests, procedures, or non-COVID diagnoses.Also applies to: 43-43
Tools
LanguageTool
[grammar] ~3-~3: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive. (TO_NON_BASE)
Context: ...ch details events that have happened to subjects. Each row in the dataframe will have a ...
Line range hint
144-144: Correct abbreviation format for consistency and correctness.- in our algorithm (ie., the start of a "window" will always be... + in our algorithm (i.e., the start of a "window" will always be...Tools
LanguageTool
[grammar] ~3-~3: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive. (TO_NON_BASE)
Context: ...ch details events that have happened to subjects. Each row in the dataframe will have a ...
Line range hint
205-205: Simplify language for clarity.- Prior to summarizing the rest of the task tree, ... + Before summarizing the rest of the task tree, ... - In the event that the subtree root timestamp is not the same as the subtree anchor timestamp... + If the subtree root timestamp is not the same as the subtree anchor timestamp...Also applies to: 241-241
Tools
LanguageTool
[grammar] ~3-~3: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive. (TO_NON_BASE)
Context: ...ch details events that have happened to subjects. Each row in the dataframe will have a ...
Line range hint
244-244: Ensure correct modal verb usage.- If the two are not the same, they will guaranteed to be separated by a constant `offset` ... + If the two are not the same, they will be guaranteed to be separated by a constant `offset` ...Tools
LanguageTool
[grammar] ~3-~3: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive. (TO_NON_BASE)
Context: ...ch details events that have happened to subjects. Each row in the dataframe will have a ...
Line range hint
257-257: Add missing commas for clarity.- a dataframe keyed by the `subject_id` column as well as by any possible prospective ... + a dataframe keyed by the `subject_id` column, as well as by any possible prospective ... - realization of this subtree's anchor event and contains a `True` value otherwise. + realization of this subtree's anchor event, and contains a `True` value otherwise.Also applies to: 286-286
Tools
LanguageTool
[grammar] ~3-~3: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive. (TO_NON_BASE)
Context: ...ch details events that have happened to subjects. Each row in the dataframe will have a ...
Line range hint
334-334: Correct preposition usage for clarity.- this timestamp can be manually specified to any start or end timestamp of any desired window + this timestamp can be manually specified as any start or end timestamp of any desired windowTools
LanguageTool
[grammar] ~3-~3: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive. (TO_NON_BASE)
Context: ...ch details events that have happened to subjects. Each row in the dataframe will have a ...
Line range hint
336-336: Correct abbreviation format for consistency and correctness.- at which point a prediction can be made (ie., at the end of the `input` windows). + at which point a prediction can be made (i.e., at the end of the `input` windows).Tools
LanguageTool
[grammar] ~3-~3: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive. (TO_NON_BASE)
Context: ...ch details events that have happened to subjects. Each row in the dataframe will have a ...
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- README.md (2 hunks)
- docs/source/configuration.md (7 hunks)
- docs/source/index.md (2 hunks)
- docs/source/notebooks/examples.ipynb (1 hunks)
- docs/source/notebooks/predicates.ipynb (3 hunks)
- docs/source/profiling.md (1 hunks)
- docs/source/terminology.md (6 hunks)
- docs/source/usage.md (2 hunks)
- src/aces/configs/init.py (1 hunks)
Files not reviewed due to errors (1)
- README.md (no review received)
Files skipped from review as they are similar to previous changes (3)
- docs/source/notebooks/predicates.ipynb
- docs/source/profiling.md
- src/aces/configs/init.py
Additional context used
LanguageTool
docs/source/index.md
[style] ~56-~56: Opting for a less wordy alternative here can improve the clarity of your writing. (NOT_ONLY_ALSO)
Context: ...ithin the ACES ecosystem. This approach not only simplifies the process but also ensures high-quality, reproducible results for ...
[style] ~56-~56: Using many exclamation marks might seem excessive (in this case: 8 exclamation marks for a text that’s 2719 characters long) (EN_EXCESSIVE_EXCLAMATION)
Context: ... datasets in reasonable raw formulations!docs/source/usage.md
[uncategorized] ~154-~154: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...To set a data standard:data.standard: String specifying the data standard, mu...
[uncategorized] ~158-~158: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...y from a single MEDS shard:data.path: Path to the.parquetshard file To qu...
[uncategorized] ~162-~162: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...ata=sharded. Additionally:data.root`: Root directory of MEDS dataset containi...
[uncategorized] ~164-~164: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...ntaining shard directoriesdata.shard: Expression specifying MEDS shards (`$(e...
[uncategorized] ~168-~168: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...uery from an ESGPT dataset:data.path: Directory of the full ESGPT dataset To...
[uncategorized] ~174-~174: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...e predicates dataframedata.ts_format: Timestamp format for predicates. Defaul...
[uncategorized] ~178-~178: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ..." #### Task Configurationcohort_dir: Directory the your task configuration f...
[grammar] ~178-~178: A determiner cannot be combined with a possessive pronoun. Did you mean simply “the” or “your”? (A_MY)
Context: ... Configurationcohort_dir: Directory the your task configuration filecohort_name:...
[uncategorized] ~180-~180: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...r task configuration filecohort_name: Name of the task configuration file Th...
[uncategorized] ~184-~184: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...ing results, and logging:config_path: Path to the task configuration file. De...
[uncategorized] ~186-~186: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: .../${cohort_name}.yamloutput_filepath`: Path to store the outputs. Defaults to ...docs/source/configuration.md
[uncategorized] ~30-~30: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...consists of three parts: -predicates, stored as a dictionary from string pred...
[uncategorized] ~33-~33: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...t build on other predicates. -trigger, stored as a string toEventConfig- `...
[uncategorized] ~43-~43: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...cateConfig####PlainPredicateConfig`: Configuration of Predicates that can be...
[uncategorized] ~47-~47: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ... of the following four fields: -code: The string value for the categorical co...
[uncategorized] ~49-~49: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...s code in the observation. -value_min: If specified, an observation will only ...
[style] ~51-~51: ‘on the basis of’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_ON_THE_BASIS_OF)
Context: ..._min(with these options being decided on the basis ofvalue_min_inclusive, wherevalue_m...
[uncategorized] ~55-~55: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ... equal to will be used). -value_max: If specified, an observation will only ...
[style] ~57-~57: ‘on the basis of’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_ON_THE_BASIS_OF)
Context: ..._max(with these options being decided on the basis ofvalue_max_inclusive, wherevalue_m...
[uncategorized] ~61-~61: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ... will be used). -value_min_inclusive: Seevalue_min-value_max_inclusive...
[uncategorized] ~90-~90: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ... column. ####DerivedPredicateConfig: Configuration of Predicates that Depend...
[uncategorized] ~95-~95: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ... -and(pred_1_name, pred_2_name, ...): Asserts that all of the specified predi...
[style] ~95-~95: Consider removing “of” to be more concise (ALL_OF_THE)
Context: ..._name, pred_2_name, ...): Asserts that all of the specified predicates must be true. -o...
[uncategorized] ~96-~96: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...e. -or(pred_1_name, pred_2_name, ...): Asserts that any of the specified predi...
[uncategorized] ~106-~106: You might be missing the article “a” here. (AI_EN_LECTOR_MISSING_DETERMINER_A)
Context: ...he predicate that must be observed with value greater than one to satisfy the event. ...
[uncategorized] ~137-~137: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...REFERENCING = $REFERENCED + $TIME_DELTA,$REFERENCING = $REFERENCED - $TIME_DEL...
[uncategorized] ~146-~146: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...REFERENCING = $REFERENCED -> $PREDICATE,$REFERENCING = $REFERENCED <- $PREDICA...
[uncategorized] ~181-~181: Possible missing preposition found. (AI_HYDRA_LEO_MISSING_OF)
Context: ...max_valid)` that define the valid range the count of observations of the named pred...docs/source/terminology.md
[grammar] ~3-~3: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive. (TO_NON_BASE)
Context: ...ch details events that have happened to subjects. Each row in the dataframe will have a ...
[style] ~8-~8: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it. (A_VARIETY_OF)
Context: ... of columns which describe the event in a variety of ways. These columns can either have a b...
[style] ~43-~43: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it. (A_VARIETY_OF)
Context: ...in this particular case could represent a variety of other events in the medical record, suc...
[grammar] ~91-~91: Did you mean “exists”? (THERE_VBP_A_NN)
Context: ...n a "valid" form for a patient if there exist a set of time points such that, within ...
[uncategorized] ~144-~144: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...s strictly increasing in our algorithm (ie., the start of a "window" will always be...
[style] ~205-~205: ‘Prior to’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_PRIOR_TO)
Context: ...#### Identify Prospective Root Anchors Prior to summarizing the rest of the task tree, ...
[style] ~241-~241: ‘In the event that’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_IN_THE_EVENT_THAT)
Context: ...ng the recursive step. #####offsetIn the event that the subtree root timestamp is not the s...
[grammar] ~244-~244: The modal verb ‘will’ requires the verb’s base form. (MD_BASEFORM)
Context: ... If the two are not the same, they will guaranteed to be separated by a constantoffset...
[uncategorized] ~257-~257: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...n a dataframe keyed by thesubject_idcolumn as well as by any possible prospective ...
[uncategorized] ~286-~286: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ... realization of this subtree's anchor event and contains aTruevalue otherwise. ...
[uncategorized] ~334-~334: The preposition ‘as’ seems more likely in this position. (AI_HYDRA_LEO_REPLACE_TO_AS)
Context: ...his timestamp can be manually specified to any start or end timestamp of any desir...
[uncategorized] ~336-~336: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...t which point a prediction can be made (ie., at the end of theinputwindows). #...README.md
[uncategorized] ~22-~22: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...yses can also become more accessible to clinicians as it enables the extraction of tailore...
[uncategorized] ~243-~243: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...eld for a "derived" predicate: -expr: Must be a string with the 'and()' or 'o...
[uncategorized] ~245-~245: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...e any other predicate manually defined (ie., setting a constraint on its occurrence...
[uncategorized] ~251-~251: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ... and can be directly used:_ANY_EVENT: specifies any event in the data (ie., e...
[uncategorized] ~251-~251: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...VENT: specifies any event in the data (ie., effectively set to1` for every singl...
[uncategorized] ~253-~253: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...r predicates dataframe)_RECORD_START: specifies the beginning of a patient's ...
[uncategorized] ~253-~253: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...es the beginning of a patient's record (ie., effectively set to1in the first ch...
[uncategorized] ~255-~255: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...w for everysubject_id)_RECORD_END: specifies the end of a patient's record...
[uncategorized] ~255-~255: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...pecifies the end of a patient's record (ie., effectively set to1in the last chr...
[style] ~271-~271: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’. (HYPHENATED_LY_ADVERB_ADJECTIVE)
Context: ...Windows Windows can be of two types, a temporally-bounded window or an event-bounded window. Belo...
[style] ~271-~271: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’. (HYPHENATED_LY_ADVERB_ADJECTIVE)
Context: ...event-bounded window. Below is a sample temporally-bounded window configuration: ``` trigger: adm...
[uncategorized] ~285-~285: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...e, the windowinputbegins atNULL(ie., the first event or the start of the ti...
[uncategorized] ~285-~285: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: .... The window is inclusive on both ends (ie., both the first event and the event at ...
[uncategorized] ~285-~285: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE_2)
Context: ...traint of 5 events of any kind is placed so any valid window would include sufficie...
[uncategorized] ~287-~287: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...tive time period expressed as a string (ie.,+ 2 days,- 365 days,+ 12h, `- ...
[uncategorized] ~287-~287: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...directional arrow and a predicate name (ie.,-> predicate_1or<- predicate_1)....
[uncategorized] ~291-~291: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...ed by usingNoneor leaving it empty (ie.,(5, None),(8,),(None, 32), `(,...
[uncategorized] ~291-~291: Commas should not be placed before a closing parenthesis. Either move the comma outside of the parentheses, or remove it altogether. (COMMA_CLOSING_PARENTHESIS)
Context: ... or leaving it empty (ie.,(5, None),(8,),(None, 32),(,10)).labelis a...
[uncategorized] ~295-~295: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ... at which the prediction would be made (ie., set toendin your window containing...
[uncategorized] ~308-~308: Possible missing preposition found. (AI_HYDRA_LEO_MISSING_TO)
Context: ...a-Standard/meds_etl), which can be used generate automated tabular baseline methods (ie....
[uncategorized] ~308-~308: The abbreviation “i.e.” (= that is) requires two periods. (I_E)
Context: ...ate automated tabular baseline methods (ie., XGBoost over ACES-defined tasks). - [M...
[style] ~317-~317: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it. (A_VARIETY_OF)
Context: ...task definitions, and can be applied to a variety of schemas, making it a versatile tool sui...
Markdownlint
docs/source/usage.md
70-70: null (MD040, fenced-code-language)
Fenced code blocks should have a language specifieddocs/source/terminology.md
126-126: Expected: h3; Actual: h4 (MD001, heading-increment)
Heading levels should only increment by one level at a time
100-100: null (MD040, fenced-code-language)
Fenced code blocks should have a language specifiedREADME.md
69-69: Punctuation: ':' (MD026, no-trailing-punctuation)
Trailing punctuation in heading
81-81: Punctuation: ':' (MD026, no-trailing-punctuation)
Trailing punctuation in heading
190-190: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
273-273: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
Additional comments not posted (7)
docs/source/index.md (1)
19-20: Renaming sections aligns with the updated focus and terminology.docs/source/usage.md (1)
64-65: Good addition of detailed explanations for configuration fields.docs/source/configuration.md (3)
1-1: Excellent introduction to the configuration language specification.
28-28: Clear explanation of the structure of the configuration file.
39-39: Detailed and informative description ofPlainPredicateConfigandDerivedPredicateConfig.Also applies to: 71-75
docs/source/notebooks/examples.ipynb (1)
240-242: Clear and concise explanation of the task configuration for imminent mortality prediction.docs/source/terminology.md (1)
3-3: Address grammatical issue in the introductory paragraph.- which details events that have happened to subjects. + detailing events that have occurred to subjects.Likely invalid or redundant comment.
Tools
LanguageTool
[grammar] ~3-~3: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive. (TO_NON_BASE)
Context: ...ch details events that have happened to subjects. Each row in the dataframe will have a ...
| # Algorithm & Design | ||
|
|
||
| ## Introduction | ||
| ## Terminology & Design |
There was a problem hiding this comment.
Update section title to maintain consistent heading levels.
- ## Terminology & Design
+ # Terminology & DesignCommittable 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.
| ## Terminology & Design | |
| # Terminology & Design |
| ``` | ||
|
|
||
| Given that our machine learning model seeks to predict in-hospital mortality, our dataset should include both | ||
| positive and negative samples (patients that died in the hospital and patients that didn't die). Hence, the | ||
| `target` "window" concludes at either a `"death"` event (patients that died) or a`"discharge"` event | ||
| (patients that didn't die). | ||
|
|
||
| Note that this is conceptual pseudocode and not the actual configuration language, which may look slightly | ||
| different. Nevertheless, we can see that this set of specifications can be realized in a "valid" form for a | ||
| We can see that this set of specifications can be realized in a "valid" form for a | ||
| patient if there exist a set of time points such that, within 48 hours after an admission, there are no |
There was a problem hiding this comment.
Correct verb form for grammatical accuracy.
- if there exist a set of time points such that, within 48 hours after an admission, there are no discharges, deaths, or COVID diagnoses
+ if there exists a set of time points such that, within 48 hours after an admission, there are no discharges, deaths, or COVID diagnosesCommittable 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.
| patient if there exist a set of time points such that, within 48 hours after an admission, there are no | |
| patient if there exists a set of time points such that, within 48 hours after an admission, there are no |
Tools
LanguageTool
[grammar] ~91-~91: Did you mean “exists”? (THERE_VBP_A_NN)
Context: ...n a "valid" form for a patient if there exist a set of time points such that, within ...
|
|
||
| - Painless Reproducibility: Use [MEDS-Tab](https://github.com/mmcdermott/MEDS_TAB_MIMIC_IV/tree/main/tasks) to obtain comparable, reproducible, and well-tuned XGBoost results tailored to your dataset-specific feature space! | ||
|
|
||
| By following these steps, you can seamlessly transform your dataset, define necessary criteria, and leverage powerful machine learning tools within the ACES ecosystem. This approach not only simplifies the process but also ensures high-quality, reproducible results for your machine learning for health projects. It can reliably take no more than a week of full-time human effort to perform steps 1-5 on new datasets in reasonable raw formulations! | ||
| By following these steps, you can seamlessly transform your dataset, define necessary criteria, and leverage powerful machine learning tools within the ACES ecosystem. This approach not only simplifies the process but also ensures high-quality, reproducible results for your machine learning for health projects. It can reliably take no more than a week of full-time human effort to perform Steps I-V on new datasets in reasonable raw formulations! |
There was a problem hiding this comment.
Consider simplifying the sentence for better readability.
- This approach not only simplifies the process but also ensures high-quality, reproducible results for your machine learning for health projects. It can reliably take no more than a week of full-time human effort to perform Steps I-V on new datasets in reasonable raw formulations!
+ This approach simplifies the process and ensures high-quality, reproducible results. Typically, Steps I-V can be completed within a week of full-time effort on new datasets.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.
| By following these steps, you can seamlessly transform your dataset, define necessary criteria, and leverage powerful machine learning tools within the ACES ecosystem. This approach not only simplifies the process but also ensures high-quality, reproducible results for your machine learning for health projects. It can reliably take no more than a week of full-time human effort to perform Steps I-V on new datasets in reasonable raw formulations! | |
| By following these steps, you can seamlessly transform your dataset, define necessary criteria, and leverage powerful machine learning tools within the ACES ecosystem. This approach simplifies the process and ensures high-quality, reproducible results. Typically, Steps I-V can be completed within a week of full-time effort on new datasets. |
Tools
LanguageTool
[style] ~56-~56: Opting for a less wordy alternative here can improve the clarity of your writing. (NOT_ONLY_ALSO)
Context: ...ithin the ACES ecosystem. This approach not only simplifies the process but also ensures high-quality, reproducible results for ...
[style] ~56-~56: Using many exclamation marks might seem excessive (in this case: 8 exclamation marks for a text that’s 2719 characters long) (EN_EXCESSIVE_EXCLAMATION)
Context: ... datasets in reasonable raw formulations!
| @@ -18,7 +18,7 @@ pip install es-aces | |||
|
|
|||
| **Example: `inhospital_mortality.yaml`** | |||
|
|
|||
| Please see the [Task Configuration File Overview](https://eventstreamaces.readthedocs.io/en/latest/overview.html#task-configuration-file) for details on how to create this configuration for your own task! More examples are available [here](https://eventstreamaces.readthedocs.io/en/latest/notebooks/examples.html). | |||
| Please see the [Task Configuration File Overview](https://eventstreamaces.readthedocs.io/en/latest/overview.html#task-configuration-file) for details on how to create this configuration for your own task! More examples are available [here](https://eventstreamaces.readthedocs.io/en/latest/notebooks/examples.html) and in the [GitHub repository](https://github.com/justin13601/ACES/tree/main/sample_configs). | |||
There was a problem hiding this comment.
Clarify the usage and purpose of the configuration file.
+ Please see the [Task Configuration File Overview](https://eventstreamaces.readthedocs.io/en/latest/overview.html#task-configuration-file) for details on how to create this configuration for your own task! More examples are available [here](https://eventstreamaces.readthedocs.io/en/latest/notebooks/examples.html) and in the [GitHub repository](https://github.com/justin13601/ACES/tree/main/sample_configs).Committable suggestion was skipped due to low confidence.
| @@ -95,7 +95,7 @@ accepted operations that can be applied to other predicates, containing precisel | |||
| - `and(pred_1_name, pred_2_name, ...)`: Asserts that all of the specified predicates must be true. | |||
| - `or(pred_1_name, pred_2_name, ...)`: Asserts that any of the specified predicates must be true. | |||
|
|
|||
| Note that, currently, `and`'s and `or`'s cannot be nested. Upon user request, we may support further advanced | |||
| **Note**: Currently, `and`'s and `or`'s cannot be nested. Upon user request, we may support further advanced | |||
There was a problem hiding this comment.
Consider future enhancements based on user feedback.
Would you like me to open a GitHub issue to track requests for nested logical operations in predicates?
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)
- docs/source/profiling.md (1 hunks)
- src/aces/configs/init.py (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- docs/source/profiling.md
- src/aces/configs/init.py
Summary by CodeRabbit