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

Added prefix argument to Grid writeTimeStep #742

Closed
wants to merge 8 commits into from
Closed

Added prefix argument to Grid writeTimeStep #742

wants to merge 8 commits into from

Conversation

DavidJoy8
Copy link
Contributor

Grid's BovWriter currently does not allow the user any control over the filename. This change makes it work the same as the HDF5 particle output.

Copy link
Member

@streeve streeve left a comment

Choose a reason for hiding this comment

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

Test failures aren't your fault - see #743

void writeTimeStep( ExecutionSpace, const int time_step_index,
const double time, const Array_t& array,
const bool gather_array = true )
void writeTimeStep( ExecutionSpace, const std::string& prefix,
Copy link
Member

Choose a reason for hiding this comment

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

We'll want a backwards compatibility wrapper that passes an empty string to this function (that we deprecate)

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 tried to create this (instead of passing an empty string, the deprecated version replicates the previous behavior of creating a prefix based on the array label). However, there's an issue stemming from there already being another version of this function that infers the ExecutionSpace:
When one (like in the test cases) calls writeTimeStep( "grid_cell_field_2d", 302, 3.43, *cell_field ), it should be going to the new, non-deprecated method, but the compiler instead matches it to the templated writeTimeStep( ExecutionSpace, const int...), templating ExecutionSpace to std::string.
I don't know how to fix this; I tried some stuff that would say "template ExecutionSpace to any class but std::string" but couldn't get it to work.

Copy link
Member

Choose a reason for hiding this comment

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

We'll just need to add an enable_if to restrict that first argument to a valid execution space - we can chat about it Monday

@DavidJoy8 DavidJoy8 marked this pull request as draft March 22, 2024 14:23
}

template <class ExecutionSpace, class Array_t>
[[deprecated( "writeTimeStep now requires a filename prefix" )]] void
Copy link
Member

Choose a reason for hiding this comment

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

Looking at this again, I would rather not deprecate and just directly use the array label as the prefix in the variant

@DavidJoy8
Copy link
Contributor Author

Messed up my repo; closing this and will create a new pull request

@DavidJoy8 DavidJoy8 closed this Mar 25, 2024
@streeve
Copy link
Member

streeve commented Mar 25, 2024

Messed up my repo; closing this and will create a new pull request

You can just reopen, git rebase master, and force push to this branch if you want

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