-
Notifications
You must be signed in to change notification settings - Fork 5
Bugfixes and documentation improvements #252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ISrecordType value updated
Co-authored-by: rix133 <[email protected]>
Co-authored-by: rix133 <[email protected]>
Only when main updates
…esdataobjects Add warning when combining RDBESDataObjects from different hierarchies
Merge pull request #244 from ices-tools-dev/dev
passed the test: #246 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request addresses bug #251 and implements several improvements to documentation, parameter handling, validation logic, and ID table creation within the RDBEScore package.
Key changes:
- Fixed data type validation to treat numeric and integer types as compatible (#251)
- Enhanced documentation for
combineRDBESDataObjects()andcreateRDBESDataObject()to clarify hierarchy handling and parameter options - Added hierarchy validation to prevent/warn when combining objects from different hierarchies
Reviewed Changes
Copilot reviewed 39 out of 41 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| R/validateRDBESDataObjectDataTypes.R | Fixed bug #251 by treating integer/numeric as compatible in both directions |
| R/combineRDBESDataObjects.R | Added hierarchy checking logic with strict parameter support |
| R/createRDBESDataObject.R | Expanded documentation for ... parameter and hierarchy selection |
| R/createTableOfRDBESIds.r | Enhanced merging logic for different lower hierarchies with improved console output |
| R/lowerTblData.R | Updated to preserve intermediate IDs in the hierarchy traversal path |
| R/importRDBESDataZIP.R | Corrected spelling from "Hierachy" to "Hierarchy" |
| man/*.Rd | Updated documentation files to reflect code changes |
| tests/testthat/*.R | Added/updated tests for data type validation and object combination |
| vignettes/*.Rmd | Added examples for inverse filtering and updated code snippets |
| data-raw/exampleData/TextBookExamples/*.R | Build scripts updated with new fields and corrected structure |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
nmprista
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at first sight, looks ok
Bugs
Bug fix: addressed #251
Enhancements to documentation and parameter handling:
combineRDBESDataObjectsandcreateRDBESDataObject, clarifying behavior when combining or importing objects with different hierarchies and how to specify which hierarchy to use. [1] [2] [3]...(ellipsis) parameters increateRDBESDataObject, providing clear guidance for users on strictness, verbosity, and hierarchy selection.Validation and warning logic for mixed hierarchies:
combineRDBESDataObjectsnow checks for and warns (or errors, ifstrict=TRUE) when combining objects from different hierarchies, preventing accidental creation of structurally invalid objects.combineRDBESDataObjectsshould be more explicit in the handling of multiple hierarchies #242Improvements to ID table creation:
createTableOfRDBESIdsnow coded in data.table, with more robust merging logic for different lower hierarchies, added informative console output, and clarified handling of special cases (e.g.,BVtable merges depending on hierarchy). [1] [2]