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

Extend Derived Variable Expressions: Trig functions #4282

Merged
merged 11 commits into from
Aug 20, 2024

Conversation

lizdulac
Copy link
Collaborator

@lizdulac lizdulac commented Aug 1, 2024

Extend Derived Variable Expressions to include sin, cos, tan, asin, acos, atan

@lizdulac lizdulac changed the title Trig fns Extend Derived Variable Expressions: Trig fns Aug 1, 2024
@lizdulac lizdulac changed the title Extend Derived Variable Expressions: Trig fns Extend Derived Variable Expressions: Trig functions Aug 1, 2024
Comment on lines 42 to 58
template <class T>
T *ApplyOneToOneOnce(T *inputData, size_t dataSize, std::function<T(T)> compFct)
{
T *outValues = (T *)malloc(dataSize * sizeof(T));
if (outValues == nullptr)
{
helper::Throw<std::invalid_argument>("Derived", "Function", "ApplyOneToOneOnce",
"Error allocating memory for the derived variable");
}
for (size_t i = 0; i < dataSize; i++)
{
T data = *(reinterpret_cast<T *>(inputData + i));
outValues[i] = compFct(data);
}
return outValues;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This function is basically a std::transform. Check how sqrt is implemented now and update the trig functions using the same method.

Copy link
Contributor

@anagainaru anagainaru left a comment

Choose a reason for hiding this comment

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

Trig functions have the same issue as pow/sqrt so the type function used should not be SameType.
Also, see my comment on ApplyOneToOneOnce

Copy link
Contributor

@anagainaru anagainaru left a comment

Choose a reason for hiding this comment

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

The new version looks good, I turned ON the derived variables to make sure all tests pass.

Later edit: all tests passed

anagainaru
anagainaru previously approved these changes Aug 19, 2024
@anagainaru anagainaru merged commit 4026559 into ornladios:master Aug 20, 2024
38 checks passed
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