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

Gb/presrat updates 2 #230

Merged
merged 17 commits into from
Sep 12, 2024
Merged

Gb/presrat updates 2 #230

merged 17 commits into from
Sep 12, 2024

Conversation

grantbuster
Copy link
Member

@grantbuster grantbuster commented Aug 27, 2024

  1. Moved a few things around to make presrat callable from a script with parameters.
  2. Lean on delta_demon_min from rex to prevent divide by small numbers in QDM
  3. window_size and n_time_steps are now kwargs so you can set at runtime. found issues with default class attrs for precip
  4. fixed zero rate threshold to actually be 0.01 mm/day and updated use to prevent QDM divide by small number

@grantbuster grantbuster marked this pull request as ready for review August 30, 2024 21:40
Copy link
Collaborator

@bnb32 bnb32 left a comment

Choose a reason for hiding this comment

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

I think this looks fine, esp for merging into the refactor branch, but we should be wary of letting the bc stuff get too bloated. Not sure if you get the same feeling that a clean up might be around the corner.

shape[1],
rasterizer.shape[2],
)
good_shape = (shape[0], shape[1], rasterizer.shape[2])
Copy link
Collaborator

Choose a reason for hiding this comment

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

lol. fair point :)

Copy link
Member Author

Choose a reason for hiding this comment

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

haha i dont even remember why i felt the need to fix this but yes trying to have less ugly one liners given the new linting.

@@ -105,6 +105,22 @@
'min': -200,
'max': 100,
},
'temperature_min': {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should probably move this dict to its own file, huh?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes could be json just need to make sure the file gets included in pip install like this: https://github.com/NREL/reV/blob/main/setup.py#L87

Want me to do this or have you done it already?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Haven't done this yet so if you could that'd be awesome.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay done, can you merge when happy?

Copy link
Collaborator

Choose a reason for hiding this comment

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

will do. thanks!

sampling='linear',
log_base=10,
n_time_steps=24,
window_size=120,
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 this class needs a few more arguments ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha too much bloat? Possibly, but for now we just need to be able to modify these from config.

@bnb32 bnb32 merged commit c692000 into bnb/dh_refactor Sep 12, 2024
10 checks passed
@bnb32 bnb32 deleted the gb/presrat_updates_2 branch September 12, 2024 22:43
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