-
Notifications
You must be signed in to change notification settings - Fork 18
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
remove inline comments #98
remove inline comments #98
Conversation
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.
Fantastic, thanks a lot for cleaning this up! I’m already pre-approving this, trusting it won’t prevent you from still writing a proper CHANGELOG.rst entry & looking through my totally relevant comments. ;)
nr_runs, nr_ts, nr_gps = targ[scen].shape | ||
nr_samples_scen = nr_runs * nr_ts | ||
X[s : s + nr_samples_scen, p] = np.tile(pred_raw[scen], nr_runs) | ||
s += nr_samples_scen | ||
else: | ||
raise ValueError("Predictors of this shape cannot be processed.") | ||
|
||
# derive y (ie array of targets) | ||
# derive y (i.e. array of targets) |
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.
Very important question: Do we follow the American or the British style in MESMER comments? Because since you turned this into a proper "i.e.", I noticed the missing comma after the expression (& learned after a quick google search, that I'm apparently used to the American style whereas you apply the British style here ^^)
@@ -81,10 +81,11 @@ def separate_hist_future(var_c, time_c, cfg): | |||
|
|||
gen = cfg.gen | |||
scens_c = list(var_c.keys()) # concatenated scens | |||
scens_f = list(map(lambda x: x.replace("h-", ""), scens_c)) # future scens | |||
scens_f = [scen.replace("h-", "") for scen in scens_c] # future scens |
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.
Ah, much less overly complicated looking. 😁
@@ -5,7 +5,7 @@ | |||
import xarray as xr | |||
|
|||
|
|||
def mask_percentage(regions, lon, lat): | |||
def mask_percentage(regions, lon, lat, **kwargs): |
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.
Did you keep this in on purpose because it makes it easier to look into details between the different masks of the different regionmask versions or did you just forget about it after adding it during our zoom chat on Monday?
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 I added this on purpose so that we have the option... It's not really necessary but shouldn't be any harm either.
Codecov Report
@@ Coverage Diff @@
## master #98 +/- ##
=========================================
Coverage ? 77.53%
=========================================
Files ? 26
Lines ? 1322
Branches ? 0
=========================================
Hits ? 1025
Misses ? 297
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
isort . && black . && flake8
CHANGELOG.rst
Moves a number of inline comments to their own line - especially if this allows to have the code on one line instead of several, e.g.:
I find this can (often) increase readability considerably. These lines are most likely a artifact of applying black.
I did this relatively fast, so there is more stuff that could be cleaned. I suggest to merge soon after #93. As the code is the same (unless I made an error) I added no tests.