-
Notifications
You must be signed in to change notification settings - Fork 10
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
Gb/presrat updates 2 #230
Conversation
…y from parameters not file
…M to prevent large delta values
… issues with small window size so increased default to 120 (seasonal correction)
… round data to zero anymore
77eab6c
to
8d8474c
Compare
There was a problem hiding this 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]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol. fair point :)
There was a problem hiding this comment.
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.
sup3r/postprocessing/writers/base.py
Outdated
@@ -105,6 +105,22 @@ | |||
'min': -200, | |||
'max': 100, | |||
}, | |||
'temperature_min': { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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 ;)
There was a problem hiding this comment.
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.
delta_demon_min
from rex to prevent divide by small numbers in QDM