Skip to content

Conversation

@mcooper12590
Copy link

@mcooper12590 mcooper12590 commented Nov 10, 2025

TYPE: enhancement

KEYWORDS: speedup, declination, code profiling

SOURCE: Max P. Cooper, Aperture Space

DESCRIPTION OF CHANGES:
When inspecting the call graph generated by valgrind/callgrind WRF-Hydro is in the subroutine CALC_DECLIN in module_NoahMP_hrldas_driver ca. 30% of the time. This is due to calculation of the date with GETH_IDTS occurring within the subroutine despite day of the year not changing within the loop over all cells. Moving the date calculations outside of the loop produces a substantial speedup (0.3% in CALC_DECLIN after).

ISSUE:

TESTS CONDUCTED: Regression and rerun of callgrind.

predeclin.pdf
postdeclin.pdf

@rcabell rcabell requested a review from Copilot November 10, 2025 18:23
Copy link
Contributor

Copilot AI left a 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 PR optimizes the calculation of solar declination by moving date-dependent computations outside of a computationally expensive loop. Previously, CALC_DECLIN was called for every grid cell and recalculated the day of year and time components from the date string on each iteration, despite these values being constant across all cells. The optimization moves these calculations outside the loop and passes the pre-computed values as parameters, reducing time spent in CALC_DECLIN from ~30% to ~0.3% according to profiling results.

Key Changes:

  • Moved day-of-year calculation (GETH_IDTS) and time parsing outside the grid cell loop
  • Modified CALC_DECLIN signature to accept pre-computed values instead of the date string
  • Eliminated redundant date parsing operations within the loop

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

REAL, PARAMETER :: DPD = 360./365.
! !ARGUMENTS:
CHARACTER(LEN=19), INTENT(IN) :: NOWDATE ! YYYY-MM-DD_HH:MM:SS
!CHARACTER(LEN=19), INTENT(IN) :: NOWDATE ! YYYY-MM-DD_HH:MM:SS
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

The commented-out parameter declaration should be removed rather than left as a comment. Keeping old code as comments reduces code clarity and can cause confusion about the function's actual interface.

Suggested change
!CHARACTER(LEN=19), INTENT(IN) :: NOWDATE ! YYYY-MM-DD_HH:MM:SS

Copilot uses AI. Check for mistakes.
@scrasmussen
Copy link
Member

@mcooper12590 Thanks for putting this together! We'll test and profile the changes this week and if things look good (no expectation that they won't) we'll merge it in.

Small note, we've been in the process of removing the authored-by comments in the code because Git's history keep's track of that. So we'll be removing the MaxC -- section of the comments but if you want to you're also welcome to

@mcooper12590
Copy link
Author

You're welcome! The loop is still like this in the Noah version, but we are only using NoahMP so I did not change it there.

I've edited the comments and removed the commented line suggested by copilot.

@scrasmussen scrasmussen force-pushed the enhancement/calc-declin-speedup branch from 18eb933 to 8543969 Compare December 4, 2025 22:00
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.

2 participants