Conversation
- Added ResamplingIndicatorWithIndexing object - Refac Indicator to keep trace of injected parameter's metadata - Refac Indicator to remove explicit mentions of "indexer"
aulemahal
commented
Nov 24, 2021
| mask = reduce(np.logical_or, miss) | ||
| # Reduce by or and broadcast to ensure the same length in time | ||
| # When indexing is used and there are no valid points in the last period, mask will not include it | ||
| mask = reduce(np.logical_or, miss).reindex_like(outs[0], fill_value=True) |
Collaborator
Author
There was a problem hiding this comment.
In the case where the last period contains no points after indexing, it doesn't appear in mask and so it was dropped from the result, which is a breaking change in a way.
However, this reindex_like trick adds complexity, but fixes the problem.
I would prefer a breaking change than this.
Ex: If you do something with a time series from Jan 1st to Dec 31st, then pass freq='AS=JUL' and indexer : month=2. The second period, is completely dropped from the mask as it contains no points. Before, it wasn't dropped, rather it was masked out.
huard
approved these changes
Nov 25, 2021
Co-authored-by: David Huard <huard.david@ouranos.ca>
huard
approved these changes
Nov 26, 2021
Pull Request Test Coverage Report for Build 1508128835
💛 - Coveralls |
aulemahal
commented
Nov 26, 2021
raquelalegre
added a commit
to UCL/xclim
that referenced
this pull request
Nov 30, 2021
…s/heat_index * 'indices/heat_index' of github.com:UCL/xclim: (42 commits) Added french metadata for heat index. Apply changes to fraction indicator [sdba] Re-write of Extreme's adjustment (Ouranosinc#914) update history Update history [Ouranosinc#931] Fix indicator for Rxxp index Add missing type for `threshold_count` Add dev notes - add indexer example - other small changes run all available tests in one call for slow builds update makefile target black, only install coveralls in tox if coveralls is needed Indexing within indicators (Ouranosinc#934) Update history finish needs doctests Remove unnecessary keep_attrs Adapt to CF conventions 1.9 - remove gregorian calendar (Ouranosinc#935) Bump version: 0.31.2-beta → 0.31.3-beta update HISTORY.rst suggested corrections Apply suggestions from code review Resampling indicator (Ouranosinc#927) ...
raquelalegre
added a commit
to UCL/xclim
that referenced
this pull request
Nov 30, 2021
…es/wetday_prop * 'indices/wetday_prop' of github.com:UCL/xclim: (46 commits) Add moi-meme as 0.32 contributor do not redefine builtin next use array_almost_equal in tests Update history Improve quantile function Apply changes to fraction indicator [sdba] Re-write of Extreme's adjustment (Ouranosinc#914) update history Update history [Ouranosinc#931] Fix indicator for Rxxp index Add missing type for `threshold_count` Add dev notes - add indexer example - other small changes run all available tests in one call for slow builds update makefile target black, only install coveralls in tox if coveralls is needed Indexing within indicators (Ouranosinc#934) Update history finish needs doctests Remove unnecessary keep_attrs Adapt to CF conventions 1.9 - remove gregorian calendar (Ouranosinc#935) Bump version: 0.31.2-beta → 0.31.3-beta ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Checklist:
number) and pull request (:pull:number) has been added.bumpversion patchhas been called on this branch.zenodo.jsonWhat kind of change does this PR introduce?
ResamplingIndicatorWithIndexing, that injects theindexerkwargs, as it was done in some "stats" function. The indexing is done in thepreprocess_and_checkfunction, so the indice (compute) receives an input where values outside the select period are masked.generic.select_timewithdoy_boundsanddate_bounds. The first takes in bounds as day-of-year integers and the second takes in dates in the month-day format (%m-%d).Does this PR introduce a breaking change?
units.rate2amountwhich converts by multiplying the rate by the timestep length, which is inferred from the coordinate itself. In case of a gap, the rate is assumed constant on the whole gap...**indexerin their signature.Other information:
An indice may add an
**indexervar_keyword and handle the time selecting itself. If the indicator is constructed fromResamplingIndicator(the one that doesn't inject this new param), the missing values handling will still understand the passed indexing parameter. This can be useful when the order between indexing and computing is important (like for #884 and many others).This means that the order is currently "wrong" for many cases: i.e. indexing is done before computation. In a way, if it is made clear what the default order is, I think it's ok to have this caveat temporarily. But should we:
**indexereverywhere and slowly implement it where it makes most sense afterwards.?