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 copy to move_and_delete #233

Merged
merged 18 commits into from
Aug 8, 2023
Merged

add copy to move_and_delete #233

merged 18 commits into from
Aug 8, 2023

Conversation

juliettelavoie
Copy link
Contributor

@juliettelavoie juliettelavoie commented Aug 4, 2023

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
    • This PR fixes #xyz
  • (If applicable) Documentation has been added / updated (for bug fixes / features).
  • (If applicable) Tests have been added.
  • This PR does not seem to break the templates.
  • HISTORY.rst has been updated (with summary of main changes).
    • Link to issue (:issue:number) and pull request (:pull:number) has been added.

What kind of change does this PR introduce?

*Many small change to help my workflow work better.

  • Adding copy to move_and_delete is useful for taking everything in a directory and adding files inside an already existing structure with other files.
  • Adding var_as_str to get_cat_attrs is useful to automatically write paths without parenthesis.
    ** Before: ('tasmin',)_CanESM5.zarr
    **After: tasmin_CanESM5.zarr`
  • Adding restrict_years to compute_indicators is useful to reproduce exactly xclim's output. I want to redo exactly somehting that was done pre-xscen. The default is still to cut the nans, but this makes it possible to keep them.
  • Add documentation to template 1.

Does this PR introduce a breaking change?

no ( if you had a workflow writting a path with the tuple, it will be broken but I doubt anyone did that).

Other information:

@juliettelavoie juliettelavoie requested a review from RondeauG August 4, 2023 17:35
@@ -5,6 +5,21 @@
# It looks if the result of a task exists in the project catalog before executing the task.
# The workflow does NOT automatically remove intermediate files. You might run out of space.


Copy link
Contributor Author

@juliettelavoie juliettelavoie Aug 4, 2023

Choose a reason for hiding this comment

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

@vindelico Does this text answer the questions you had about the config ?
Is there more that should be included ?

As I said in #232, adding every possible arguments to functions in sections that are automatically passed seems overkill to me. You don't normally pass argument with just their defaults...

Copy link
Collaborator

@RondeauG RondeauG left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -193,7 +200,7 @@ def _infer_freq_from_meta(ind):
if (out[c].attrs != ds[c].attrs) and (out[c].sizes == ds[c].sizes):
out[c].attrs = ds[c].attrs

if "time" in out.dims:
if restrict_years and "time" in out.dims:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a test? What happens with something like AS-JUL ?

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 added a test with as-jul to show.
Basically, with xclim, you have a nan in the first and the last position.
The first time is the year before the start of the input.
The last time is the same year as the end of the input.
With restrict_years= False, we keep the time steps for the 2 nans.
With restrict_years= True, we cut the first nan.

@@ -1,14 +1,43 @@
# example of a config for the workflow
# Example of a config file for the workflow
Copy link
Collaborator

Choose a reason for hiding this comment

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

@juliettelavoie
I added quite some detail here.
My suggestion would be to comment the first task similarly detailed. After that, subsequent tasks all follow a similar scheme of iteration loops, anyway.

@juliettelavoie
Copy link
Contributor Author

For comments on template docs, see PR#235.
If there is no more comments on the rest, I will merge.

@RondeauG
Copy link
Collaborator

RondeauG commented Aug 8, 2023

For comments on template docs, see PR#235. If there is no more comments on the rest, I will merge.

Fine with me!

@juliettelavoie juliettelavoie merged commit 2a34733 into main Aug 8, 2023
@juliettelavoie juliettelavoie deleted the improve_move_delete branch August 8, 2023 13:34
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.

3 participants