Skip to content
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

Fix/solar zenith time cast #2098

Merged
merged 6 commits into from
Mar 5, 2025

Conversation

jack-ktw
Copy link
Contributor

@jack-ktw jack-ktw commented Mar 5, 2025

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features)
    • Note: The bug was discovered during parallel processing of multiple datasets, and appeared randomly.
    • (If applicable) Documentation has been added / updated (for bug fixes / features)
  • CHANGELOG.rst has been updated (with summary of main changes)
    • Link to issue (:issue:2097) and pull request (:pull:2098) has been added

What kind of change does this PR introduce?

Bug fix: Correct parameter order in typing.cast() causing intermittent errors in solar_zenith_angle calculation

Does this PR introduce a breaking change?

No

Other information:

his fix corrects a parameter ordering issue in the typing.cast() function call which was causing intermittent errors when processing multiple climate datasets in parallel. The fix is minimal and simply swaps the parameters to match the correct typing.cast(type, value) signature.

jack-ktw added 3 commits March 4, 2025 23:20
The cast() function in the cosine_of_solar_zenith_angle function had incorrect argument order, placing the np.ndarray type before the value to cast. This fixes the order to cast(time.values, np.ndarray) to correctly type-cast the CFTime values before converting to seconds.
Add name to AUTHORS.rst as per contribution guidelines.
Copy link

github-actions bot commented Mar 5, 2025

Welcome, new contributor!

It appears that this is your first Pull Request. To give credit where it's due, we ask that you add your information to the AUTHORS.rst and .zenodo.json:

  • The relevant author information has been added to AUTHORS.rst and .zenodo.json

Please make sure you've read our contributing guide. We look forward to reviewing your Pull Request shortly ✨

@github-actions github-actions bot added the docs Improvements to documenation label Mar 5, 2025
Copy link
Collaborator

@aulemahal aulemahal 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 to me!
Weird that we didn't stumble on this bug earlier, it simply doesn't work with cftime objects!

@Zeitsperre Zeitsperre added the approved Approved for additional tests label Mar 5, 2025
@coveralls
Copy link

coveralls commented Mar 5, 2025

Coverage Status

coverage: 89.953% (+0.009%) from 89.944%
when pulling 756e913 on jack-ktw:fix/solar-zenith-time-cast
into 66fcfaa on Ouranosinc:main.

@aulemahal
Copy link
Collaborator

@jack-ktw I allowed myself to add a test that would have detected the issue.

@aulemahal aulemahal merged commit 663e48a into Ouranosinc:main Mar 5, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for additional tests docs Improvements to documenation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Incorrect parameter order in typing.cast() causes intermittent errors in cosine_of_solar_zenith_angle
4 participants