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

Convenience kwargs in JLD2OutputWriter constructor for averaging output #887

Merged
merged 5 commits into from
Sep 3, 2020

Conversation

glwagner
Copy link
Member

@glwagner glwagner commented Aug 30, 2020

This PR adds the keyword arguments time_averaging_window and time_averaging_stride to the constructors for JLD2OutputWriter and NetCDFOutputWriter. A non-nothing time_averaging_window transforms all the specified output to a WindowedTimeAverage.

I have implemented this feature for JLD2OutputWriter only so far. @ali-ramadhan, can you confirm that the pattern applied to JLD2OutputWriter will work for NetCDFOutputWriter? I'm not 100% sure how it would work with the "slice" functionality of the NetCDFOutputWriter.

Todo:

  • [ ] Implement convenience kwargs for NetCDFOutputWriter
  • Tests for JLD2OutputWriter
  • [ ] Tests for NetCDFOutputWriter

Edit: I think we should just get this working for the JLD2OutputWriter for now. The NetCDFOutputWriter needs a bit of work anyways. We can extend NetCDFOutputWriter to averaged output once that preliminary work is complete.

@glwagner
Copy link
Member Author

Ah, it looks like I accidentally branched this PR from glw/diagnostic-dependencies rather than master. We should merge #886 first anyways.

@glwagner glwagner changed the title Convenience kwargs in OutputWriter constructors for averaging output Convenience kwargs in JLD2OutputWriter constructor for averaging output Sep 2, 2020
@codecov
Copy link

codecov bot commented Sep 2, 2020

Codecov Report

Merging #887 into master will increase coverage by 0.05%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #887      +/-   ##
==========================================
+ Coverage   71.34%   71.39%   +0.05%     
==========================================
  Files         187      187              
  Lines        5276     5289      +13     
==========================================
+ Hits         3764     3776      +12     
- Misses       1512     1513       +1     
Impacted Files Coverage Δ
src/OutputWriters/jld2_output_writer.jl 91.52% <83.33%> (-0.93%) ⬇️
test/test_output_writers.jl 93.29% <100.00%> (+0.13%) ⬆️

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 edcedc3...59d0e22. Read the comment docs.

Copy link
Member

@ali-ramadhan ali-ramadhan 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 merge! I agree with needing to refactor the NetCDFOutputWriter after which we can add time averaging capabilities to it.

# Convert each output to WindowedTimeAverage if time_averaging_window is specified
if !isnothing(time_averaging_window)

!isnothing(iteration_interval) && error("Cannot specify iteration_interval with time_averaging_window.")
Copy link
Member

Choose a reason for hiding this comment

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

I might throw an ArgumentError as it's more specific and allows us to @test_throws the constructor.

@glwagner glwagner merged commit 84641ef into master Sep 3, 2020
@glwagner glwagner deleted the glw/averaged-output branch September 8, 2020 17:37
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