Config language updates and doc #29
Merged
Conversation
This file contains 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
…d ESGPT to the PredicateConfig object
…ed into the appropriate endpoint expressions to facilitate object oriented tree building.
…s, which also includes tree building.
|
Important Auto Review SkippedAuto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 (
|
justin13601
approved these changes
May 20, 2024
justin13601
added a commit
that referenced
this pull request
May 22, 2024
* Updated summarize API to simplify summarize_window so it can be ignored in testing. * Split summarize_temporal_window into two functions; added namedtuple for type checking to aggregate function, aggregate function not working... * Fix the odd test error. This fix is very strange. * Expanded aggregate's doctest to cover other edge cases. * Added a (currently unused) named tuple for event bound parameters; added a (currently failing) doctest for event bound summarizer * updated docstrings and fixed doctest given proper understand of summarize event bound window; many more tests are needed. * added another test and marginal refinement to the summarize_event_bound docstring. * Corrected small typo, used ToEventWindowBound in summarizer. * Added a terminology description file. * Updated aggregate_temporal_window tests. * Some edits * Edits to the terminology * Small edits to the recursion description * Added a set of doctest cases for an aggregate event bound window function. * Got aggregate_event_bound working for all cases where offset is 0 * With offset doctest partially passing; need to subtract temporal group by stage as well. * Got it working in the case with a positive offset too. * Separating out functions a bit and adding doctests for check_constraints. * Added forggoten files. * Include timestamp_at_start in aggregate functions to future proof and to make property testing easier. * Some in-progress updates. * Corrected some more of the doctests. * Fixed the doctests up to the case with offset * Fixed at least one test case's worth of the with offset period. * Found a bug in the aggregate function; test is failing where (I believe) the test case has the correct output. We are likely double subtracting the row's counts through the offset aggregation and the cum_sum * Yep, it was a double subtraction issue. Fixed the error. * Another, I think, proper failing test case. * Corrected that issue too. * Another test failure; likely the counterpart to the prior one. * Corrected one more bug. It was a slightly different issue than I anticipated. * Corrected the issue. * Added another passing test case. * All test cases passing! * Removed malformed constraint checking test case and corrected typo in extract subtree doctest. * Trying to eliminate the other event bound functions to use the new general aggregation function; some test differences that warrant further assessment * Ok, this change reflects a change in API -- basically, stating that if we have an event bound window that is right inclusive, then events that are the bounding event will end up with singleton row windows. That is fine, and in my opinion better than the alternative, but it is slightly different. * Added a tiny additional comment pointing to additional testing in the event bound aggregation function. * extract subtree (partial test only) appears to be working. * Added more tests for extract_subtree. It is a little awkward as sometimes start and end are swapped unexpectedly, but the tests do seem to pass. * Removed old API files and functions. Tests are passing but query_subtree may not be. * Removing duplicated tests. * fixed query script * Config language updates and doc (#29) * Some starting code for config file updates * Some precommit fixes and documentation updates * Added basic implementation of the polars expression logic for MEDS and ESGPT to the PredicateConfig object * Expanded tests and added event_type case. * Started filling out more of the logic in the configuration objects. still in progress * More of the logic in the configuration objects. still in progress * Added doctests and more window logic so windows can naturally be parsed into the appropriate endpoint expressions to facilitate object oriented tree building. * Added (yet untested) code for initialization and validation of windows, which also includes tree building. * Added some basic doctests to the config class. * Removed references to the simplified parsing language. * Working pipeline * Remove outdated unit test file * Fix whitespace? * didn't work, undo - ask matthew * Run script working on ESGPT / CSV / presumably MEDS parquet * Update sample configs --------- Co-authored-by: Justin Xu <justin13601@hotmail.com>
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.
Updates and tests for the configuration file language and tree building. For integration into the minorly revised API in the #28