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

include dtype in np.random.uniform addressing #175 #176

Merged
merged 4 commits into from
Jan 26, 2022
Merged

Conversation

delgadom
Copy link
Member

@delgadom delgadom commented Jan 26, 2022

In this PR:

  • Tiny patch, setting dtype of np.random.uniform to match variable in wet day frequency correction

@delgadom delgadom requested review from dgergel and brews January 26, 2022 20:14
Copy link
Member

@dgergel dgergel left a comment

Choose a reason for hiding this comment

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

LGTM @delgadom but can you also add a changelog entry documenting the bug fix?

@brews
Copy link
Member

brews commented Jan 26, 2022

Thanks for the quick turn-around @delgadom. Agreed with Diana, but just add this PR number and @ your username at the end of the

Fix the wetday frequency correction so that different replacement values are used, rather than a single one (PR #174, @emileten)

line in CHANGELOG. We don't need a new entry because Emile's change hasn't been released.

I want to check to see if this still works in one other weird case we had with regridding... I'll follow up.

@brews brews added the bug Something isn't working label Jan 26, 2022
Copy link
Member

@brews brews left a comment

Choose a reason for hiding this comment

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

Thanks @delgadom! Appreciate the turnaround on this.

My only suggestion is like what Diana says, please update the CHANGELOG to append your name and this PR number onto Emile's entry for PR #174. Like (PR #174, PR #176, @emileten, @delgadom ). I don't feel like this needs an entirely new CHANGELOG entry.

Aside from that, I think this is ready to merge when you're ready.

@brews brews self-assigned this Jan 26, 2022
@brews
Copy link
Member

brews commented Jan 26, 2022

I'll update the changelog as reviews suggested and merge.

Thanks for the quick help everyone!

@brews brews merged commit 42d7177 into main Jan 26, 2022
@brews brews deleted the patch-issue-175 branch January 26, 2022 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

correct-wetday-frequency casts all output to float64
3 participants