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

Tests for splitting output files using TimeInterval #3523

Merged
merged 12 commits into from
Mar 28, 2024

Conversation

josuemtzmo
Copy link
Collaborator

@josuemtzmo josuemtzmo commented Mar 26, 2024

Adds tests for splitting output based on time-interval, e.g.,

ow = JLD2OutputWriter(model, (; u);
                      filename = "test.jld2",
                      ...
                      file_splitting = TimeInterval(3seconds)
                      )

@navidcy navidcy changed the title PR to implement and include tests for splitting output files using time. Implement and include tests for splitting output files using time Mar 26, 2024
@glwagner
Copy link
Member

We should definitely get TimeInterval to work. This is important! Why doesn't it work? I don't understand the reason.

Copy link
Member

@glwagner glwagner left a comment

Choose a reason for hiding this comment

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

Let's get TimeInterval to work.

@josuemtzmo
Copy link
Collaborator Author

We should definitely get TimeInterval to work. This is important! Why doesn't it work? I don't understand the reason.

Yes, I was also surprised. I retried and now it passed the tests, so I guess at some point I was doing something weird with TimeInterval, but I'm not sure what it was. It seems to work now.

@glwagner
Copy link
Member

Awesome! And to be more clear, I think the reason we want it to work is so that we can also reuse all the other schedules. Another reason to use it is because we may need to change TimeInterval in the future if we support different time types (for example DateTime, or other time types that solve the annoying problem of rounding error). So in all those cases it will be nice not to have to worry about different implementations of time interval schedules.

Copy link
Collaborator

@navidcy navidcy left a comment

Choose a reason for hiding this comment

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

lgtm

@navidcy navidcy requested a review from glwagner March 27, 2024 17:22
@navidcy
Copy link
Collaborator

navidcy commented Mar 27, 2024

@josuemtzmo could you update the first PR comment to reflect the latest status? As far as I can see there was not any new type introduced; just the tests, right?

@navidcy navidcy changed the title Implement and include tests for splitting output files using time Tests for splitting output files using TimeInterval Mar 27, 2024
@navidcy navidcy added the testing 🧪 Tests get priority in case of emergency evacuation label Mar 27, 2024
Copy link
Member

@glwagner glwagner left a comment

Choose a reason for hiding this comment

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

awesome contribution! I agree with @navidcy's comments but except for the cosmetic stuff it looks good

@navidcy
Copy link
Collaborator

navidcy commented Mar 28, 2024

Great! When all tests pass you can merge.

@josuemtzmo josuemtzmo merged commit 4367d42 into main Mar 28, 2024
48 checks passed
@josuemtzmo josuemtzmo deleted the jmm/time_split_output branch March 28, 2024 12:31
@navidcy
Copy link
Collaborator

navidcy commented Mar 28, 2024

excellent @josuemtzmo !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
output 💾 testing 🧪 Tests get priority in case of emergency evacuation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants