Skip to content

Conversation

@tliddell4
Copy link
Collaborator

Issue 49 Documentation updates:

  • Removed comment in readme that said "for now, data must start at midnight and be hourly" to reflect updated temporal flexibility
  • Clarified documentation for time_interval argument of staggregate_*
    Issue 50 Fix Projection in Secondary Weights
  • Within secondary_weights(), change terra::crs() to terra::project(), make reprojecting conditional on whether crs of grid and secondary_raster match, and add a message to the user that reprojecting may affect data
  • Wrote a test in test-secondary_weights.R to make sure data wasn't too distorted by projection handling

@tliddell4 tliddell4 requested a review from traceybit June 29, 2025 20:27
Copy link
Collaborator

@traceybit traceybit left a comment

Choose a reason for hiding this comment

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

@tliddell4 these changes look good to me! i left one comment for you to look at, but i don't think it's critical.


message(crayon::red("Warning: reprojecting secondary_raster to match grid, which may affect data"))
secondary_raster <- terra::project(secondary_raster, clim_raster)
terra::crs(secondary_raster) <- terra::crs(clim_raster, TRUE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think that terra::project() should automatically change the label (this seemed to happen when i tested the code), but i don't think it would lead to any errors? maybe unless the label was incorrect from the beginning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe you are correct - I've removed the crs line and pushed the change.

@traceybit
Copy link
Collaborator

merging!

@traceybit traceybit closed this Jun 30, 2025
@traceybit traceybit reopened this Jun 30, 2025
Copy link
Collaborator

@traceybit traceybit left a comment

Choose a reason for hiding this comment

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

looks good!

@tliddell4 tliddell4 merged commit 8151a8a into main Jun 30, 2025
4 checks passed
@tliddell4 tliddell4 deleted the 50-fix_projection branch July 1, 2025 21:37
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.

3 participants