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 wrapper for csg #286

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add wrapper for csg #286

wants to merge 4 commits into from

Conversation

JGuetschow
Copy link
Contributor

@JGuetschow JGuetschow commented Nov 4, 2024

Pull request

This PR adds a wrapper around the primap2.csg.compose() function that prepares input data (e.g. remove unnecessary data) and postprocesses the output (add coordinates that were used in the process with new values).

A larger test for the compose function as well a small test for a subfunction are added as well.

Docs will be extended.

Please confirm that this pull request has done the following:

  • Tests added
  • Documentation added (where applicable)
  • Description in a {pr}.thing.md file in the directory changelog added - see changelog/README.md for details

Description

Please provide a short description what your pull request does.

Copy link

codecov bot commented Nov 4, 2024

Codecov Report

Attention: Patch coverage is 98.52941% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.00%. Comparing base (ce5362a) to head (e719590).
Report is 33 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
primap2/csg/_wrapper.py 96.42% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #286      +/-   ##
==========================================
+ Coverage   96.96%   97.00%   +0.03%     
==========================================
  Files          49       51       +2     
  Lines        4612     4671      +59     
==========================================
+ Hits         4472     4531      +59     
  Misses        140      140              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JGuetschow JGuetschow requested a review from mikapfl November 5, 2024 09:09
@JGuetschow JGuetschow self-assigned this Nov 5, 2024
@JGuetschow JGuetschow added this to the Composite Source Generator milestone Nov 5, 2024
@JGuetschow JGuetschow marked this pull request as ready for review November 5, 2024 09:10
@JGuetschow JGuetschow mentioned this pull request Nov 5, 2024
3 tasks
# set time range according to input
if time_range is not None:
input_ds = input_ds.pr.loc[
{"time": pd.date_range(time_range[0], time_range[1], freq="YS", inclusive="both")}
Copy link
Member

Choose a reason for hiding this comment

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

I don't really like the very limited time range functionality. Maybe just leave it out or explicitly call it start_year and end_year, so people don't expect anything more fancy? The problem is otherwise that the time_range name is already taken in the public API, and when we need the possibility to filter for months or exclude the year 2020 or something, it will be awkward to include the new functionality. With the names start_year and end_year, we can then add a time_range parameter which takes a pd.date_range object directly or whatever works for us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought we can make it more versatile later. But I think I can do that now because I've spent the last two weeks fighting with time ranges in xarray, so It should be a quick fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would limit the option to slice and datetime index. When using the datetime index users would have to make sure the values actually exist.
For the composite source generator the only use case I can think of is a range though. But it's good if it's more versatile than just start and end year and e.g. the interval can be specified.

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