Skip to content

Tr/add thinning function#113

Open
imreddyTeja wants to merge 2 commits intomainfrom
tr/add-thinning-function
Open

Tr/add thinning function#113
imreddyTeja wants to merge 2 commits intomainfrom
tr/add-thinning-function

Conversation

@imreddyTeja
Copy link
Copy Markdown
Member

@imreddyTeja imreddyTeja commented Feb 28, 2025

Proposed function to close #86

Add thin_NCDataset!(ds_out, ds_in, thinning_factor=6, dims...)

which thins NCDataset files along the provided dimensions.

Please suggest a better interface if you have any in mind.

I'm not sure if this should edit all the Manifest files.

@imreddyTeja imreddyTeja force-pushed the tr/add-thinning-function branch from ee97383 to 68fd3a0 Compare February 28, 2025 20:18
@imreddyTeja imreddyTeja requested a review from ph-kev February 28, 2025 20:24
@imreddyTeja imreddyTeja marked this pull request as ready for review February 28, 2025 20:24
If no dimensions are provided, all dimensions are thinned.
"""
function thin_NCDataset!(ds_out, ds_in, thinning_factor=6, dims...)
all(in.(dims, Ref(keys(ds_in.dim)))) || error("Not all of $dims are in the dataset")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why we need a Ref here? Can you add a comment explaining this or find an alternative solution?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ref prevents broadcasting over the list of dimensions in the input file. I think this is done pretty commonly.

end

"""
thin_NCDataset!(ds_out, ds_in, thinning_factor=6, dims...)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you add documentation that briefly said that ds_out and ds_in are NCDatasets since they can also be confused for file paths?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also, does this copy everything beside the variable and dimensions too? I am a little confused on how this is supposed to be used.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Do you think it would be better to take in file paths?

What do you mean by "everything beside the variable and dimensions"? As in the global attributes?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think it matter as long as it is apparent that it takes in NCDataset objects or file paths.

For the second question, I am wondering if the function also copy global attributes.

@imreddyTeja imreddyTeja force-pushed the tr/add-thinning-function branch from 68fd3a0 to 6f28d28 Compare March 28, 2025 18:37
Add ` thin_NCDataset!(ds_out, ds_in, thinning_factor=6, dims...)`

which thins NCDataset files along the provided dimensions.
@imreddyTeja imreddyTeja force-pushed the tr/add-thinning-function branch from 6f28d28 to c85252c Compare March 28, 2025 18:38
@imreddyTeja imreddyTeja requested a review from ph-kev March 28, 2025 18:39
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.

Move function to reduce size of artifacts to ClimaArtifactsHelper

2 participants