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

Add support for group-by financial years. #1257

Merged
merged 5 commits into from May 23, 2022
Merged

Add support for group-by financial years. #1257

merged 5 commits into from May 23, 2022

Conversation

tebadi
Copy link
Contributor

@tebadi tebadi commented May 19, 2022

Reason for this pull request

Support virtual products to group-by financial years.

Proposed changes

  • Added a new function to support this alongside the calendar year.

  • Closes #xxxx

  • Tests added / passed

  • Fully documented, including docs/about/whats_new.rst for all changes

@codecov
Copy link

codecov bot commented May 19, 2022

Codecov Report

Merging #1257 (8394289) into develop (67dde96) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #1257      +/-   ##
===========================================
- Coverage    93.60%   93.58%   -0.03%     
===========================================
  Files          129      129              
  Lines        13145    13331     +186     
===========================================
+ Hits         12305    12476     +171     
- Misses         840      855      +15     
Impacted Files Coverage Δ
datacube/virtual/transformations.py 80.38% <100.00%> (+0.98%) ⬆️
datacube/config.py 100.00% <0.00%> (ø)
datacube/virtual/impl.py 85.80% <0.00%> (+1.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67dde96...8394289. Read the comment docs.

Copy link
Contributor

@SpacemanPaul SpacemanPaul left a comment

Choose a reason for hiding this comment

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

Looks good Toktam.

Please add a line to docs/about/whats_new.rst describing the change and referencing this PR.

"""
df = pd.Series(time.values)
years = df.apply(lambda x: np.datetime64(str(x.to_period('Q-JUN').qyear))).values
ds = xr.DataArray(years, name='time', attrs=time.attrs, coords=time.coords, dims=time.dims)
Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me too.

What happens if instead of the two lines defining and modifying ds you try to do it in one go?

ds = xr.DataArray(years, name='time', attrs=time.attrs, coords=years, dims=time.dims)

I think that should work!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The coordinates should be a dictionary so something like below will work but I'm not 100% sure if the coords can contain other variables than time which the following code will exclude:

ds = xr.DataArray(years, name='time', attrs=time.attrs, coords=({"time": years}), dims=time.dims)

Copy link
Contributor

Choose a reason for hiding this comment

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

How about writing a test - that way we know for sure if it works the way we expect. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Most of the existing time grouping transforms aren't covered by tests either, but they are trivial compared to this one.

@SpacemanPaul SpacemanPaul self-requested a review May 23, 2022 23:26
Copy link
Contributor

@SpacemanPaul SpacemanPaul left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks Toktam.

@SpacemanPaul SpacemanPaul merged commit 78867da into develop May 23, 2022
@SpacemanPaul SpacemanPaul deleted the group_by_fy branch May 23, 2022 23:55
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.

None yet

3 participants