Skip to content

fixes data1#1006

Merged
Bai-Li-NOAA merged 2 commits into
dev-json-caa-rebased-BaiLifrom
dev-json-caa-rebased-BaiLi-CodeClub
Oct 2, 2025
Merged

fixes data1#1006
Bai-Li-NOAA merged 2 commits into
dev-json-caa-rebased-BaiLifrom
dev-json-caa-rebased-BaiLi-CodeClub

Conversation

@kellijohnson-NOAA

Copy link
Copy Markdown
Contributor

We will be using timing from now on instead of date where timing is a double which allows for the year to be incremented using whatever resolution you input. Right now, the only resolution that is available in FIMS is year so timing must be an integer value.

What is the feature?

  • Simplified data1

How have you implemented the solution?

  • Changed datestart to timing
  • Removed dateend

Does the PR impact any other area of the project, maybe another repo?

Part of #982

@Bai-Li-NOAA I have listed you as a reviewer because I would like to merge this into your PR that goes into dev-json-caa and then eventually dev.

@github-actions

github-actions Bot commented Oct 1, 2025

Copy link
Copy Markdown
Contributor

Instructions for code reviewer

Hello reviewer, thanks for taking the time to review this PR!

  • Please use this checklist during your review, checking off items that you have verified are complete, but feel free to skip over items that are not relevant!
  • See the GitHub documentation for how to comment on a PR to indicate where you have questions or changes are needed before approving the PR.
  • Please use conventions in the guidelines for conventional commit messages for both commit messages and comments.
  • PR reviews are a great way to learn so feel free to share your tips and tricks. However, when suggesting changes to the PR that are optional please include nit: (for nitpicking) as the comment type. For example, nit: I prefer using a data.frame() instead of a matrix because ...
  • Engage with the developer when they respond to comments and check off additional boxes as they become complete so the PR can be merged in when all the tasks are fulfilled. Make it clear when the PR is approved by selecting the approved status, and potentially by commenting on the PR with something like This PR is now ready to be merged, no additional changes are needed.

Checklist

  • The PR is requested to be merged into the appropriate branch (typically dev).
  • The code is well-designed.
  • The functionality is good for the users of the code.
  • Code coverage remains high, indicating the new code is tested.
  • The code is commented and the comments are clear, useful, and mostly explain why instead of what.
  • Code is appropriately documented (doxygen and roxygen).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR simplifies the data structure by replacing date-based timing with numeric timing values. It changes from using datestart and dateend columns to a single timing column that represents years as numeric values, enabling future support for fractional year resolution while currently restricting to integer values.

  • Replaced date columns (datestart, dateend) with single numeric timing column
  • Updated validation logic to check for numeric timing instead of date format validation
  • Modified data generation and processing functions to use numeric years directly

Reviewed Changes

Copilot reviewed 5 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
inst/WORDLIST Removed dateend and datestart entries, added timing and timings
data-raw/data1.R Replaced date creation logic with direct year assignment from timing column
R/initialize_modules.R Updated global variables and column references from date columns to timing
R/fimsframe.R Refactored validation, plotting, and data processing to use numeric timing instead of dates
R/data1.R Updated documentation to reflect new timing column structure

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread data-raw/data1.R Outdated
Comment thread data-raw/data1.R Outdated
@Bai-Li-NOAA Bai-Li-NOAA force-pushed the dev-json-caa-rebased-BaiLi branch 3 times, most recently from c6acadb to 3a8beb0 Compare October 2, 2025 14:39
Bai-Li-NOAA and others added 2 commits October 2, 2025 14:43
- add get_finalized_fims to FIMSFit class
- add finalized_fims slot to FIMSFit class
- add a test for get_finalized_fims()
- add a test for reshape_json_estimates() to address
  issue #898
- update -999 or NA to null for JSON output
  since JSON has a dedicated literal for missing
  values. The {jsonlite} package will automatically
  convert null in JSON to NA in R.
- We will be using timing from now on instead of date where timing is a
double which allows for the year to be incremented using whatever
resolution you input. Right now, the only resolution that is available
in FIMS is year so timing must be an integer value.
- fix(fimsframe): Fixes bug in FIMSFrame validators

Co-authored-by: Dr. Wilson <awilnoaa@users.noreply.github.com>
Co-authored-by: alexjensen-NOAA <alexjensen-NOAA@users.noreply.github.com>
@Bai-Li-NOAA Bai-Li-NOAA force-pushed the dev-json-caa-rebased-BaiLi-CodeClub branch from ff1f945 to 428a543 Compare October 2, 2025 14:49
@Bai-Li-NOAA Bai-Li-NOAA merged commit a878ef5 into dev-json-caa-rebased-BaiLi Oct 2, 2025
5 of 9 checks passed
@Bai-Li-NOAA Bai-Li-NOAA deleted the dev-json-caa-rebased-BaiLi-CodeClub branch October 2, 2025 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Developer Issue]: Change datestart in data to timing_start and remove dateend

3 participants