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.

Sign up for GitHub

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

Jump to bottom

User defined functions for history outputs #1705

Merged
merged 16 commits into from
Aug 7, 2022
Merged

User defined functions for history outputs #1705

merged 16 commits into from
Aug 7, 2022

Conversation

pcarruscag
Copy link
Member

@pcarruscag pcarruscag commented Jul 11, 2022 •
edited
Loading

Proposed Changes

Allows this type of "language" to create new history outputs.

CUSTOM_OUTPUTS= 'mach : Macro{hypot(VELOCITY_X, VELOCITY_Y) / SOUND_SPEED};\
                 avg_mach : AreaAvg{$mach}[inlet];\
                 var_mach : AreaAvg{pow($mach - avg_mach, 2)}[inlet];\
                 dev_mach : Function{sqrt(var_mach)}'

Related Work

#1478

PR Checklist

  • I am submitting my contribution to the develop branch.
  • My contribution generates no new compiler warnings (try with the '-Wall -Wextra -Wno-unused-parameter -Wno-empty-body' compiler flags, or simply --warnlevel=2 when using meson).
  • My contribution is commented and consistent with SU2 style.
  • I have added a test case that demonstrates my contribution, if necessary.
  • I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp) , if necessary.

@pr-triage pr-triage bot added the PR: draft label Jul 11, 2022
@lgtm-com
Copy link

lgtm-com bot commented Jul 11, 2022

This pull request fixes 1 alert when merging 78f98e7 into 3a9f8d6 - view on LGTM.com

fixed alerts:

  • 1 for Comparison of narrow type with wide type in loop condition

@TobiKattmann
Copy link
Contributor

Even usage of custom-outputs in the custom outputs 😮

With Area/Avg+Int and Massflow-stuff, multiple markers in the list and everything I can dream of... solely CodeFactor tries to blame you for additional spaces in the python scripts :)

pcarruscag
pcarruscag commented Jul 16, 2022
View reviewed changes
TestCases/user_defined_functions/lam_flatplate.cfg
Comment on lines +31 to +34
CUSTOM_OUTPUTS= 'velocity : Macro{sqrt(pow(VELOCITY_X, 2) + pow(VELOCITY_Y, 2) + pow(VELOCITY_Z, 2))};\
avg_vel : AreaAvg{$velocity}[z_minus, z_plus];\
var_vel : AreaAvg{pow($velocity - avg_vel, 2)}[z_minus, z_plus];\
dev_vel : Function{sqrt(var_vel) / avg_vel}'
Copy link
Member Author

Choose a reason for hiding this comment

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

MVP is live if anyone wants to play.

SU2_CFD/include/output/CFlowOutput.hpp Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Jul 16, 2022

This pull request fixes 1 alert when merging 6995c81 into f2add99 - view on LGTM.com

fixed alerts:

  • 1 for Comparison of narrow type with wide type in loop condition

@pcarruscag pcarruscag changed the title [WIP] User defined functions for history outputs User defined functions for history outputs Jul 24, 2022
@pcarruscag pcarruscag marked this pull request as ready for review July 24, 2022 02:57
@pcarruscag pcarruscag requested a review from TobiKattmann July 24, 2022 02:57
@lgtm-com
Copy link

lgtm-com bot commented Jul 24, 2022

This pull request fixes 1 alert when merging f1bb97a into eed1e67 - view on LGTM.com

fixed alerts:

  • 1 for Comparison of narrow type with wide type in loop condition

@lgtm-com
Copy link

lgtm-com bot commented Jul 24, 2022

This pull request fixes 1 alert when merging 3babe42 into eed1e67 - view on LGTM.com

fixed alerts:

  • 1 for Comparison of narrow type with wide type in loop condition

@TobiKattmann
Copy link
Contributor

I'll give this a test and put in my review Friday morning 👍 :)

TobiKattmann
TobiKattmann approved these changes Jul 29, 2022
View reviewed changes
Copy link
Contributor

@TobiKattmann TobiKattmann left a comment

Choose a reason for hiding this comment

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

SEL (SU2 Expression language) seems pretty powerful and renders quite a bit of output routines and OF logic in the code unnecessary (if I see that correctly). But not only that, but it allows for custom output (no shoot sherlock) that otherwise would have required additional coding.

So big kudos and thanks a lot 💐 looks good to me (I might have struggled (and still do) to understand the code :) and works for what i tested 👍

Common/src/CConfig.cpp Outdated Show resolved Hide resolved
SU2_CFD/include/output/CFlowOutput.hpp Outdated Show resolved Hide resolved
SU2_CFD/include/output/COutput.hpp
std::vector<unsigned long> varIndices;

/*--- Offset between varIndices of different solvers (see above). Power of 2 to make decoding faster. ---*/
static constexpr unsigned long MAX_VARS_PER_SOLVER = 32;
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets see when the first person needs to solve for 33 species together with some fancy custom output :) (never... I guess)

Copy link
Member Author

Choose a reason for hiding this comment

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

It can be any 2^n, we are not allocating anything it's just an "encoding" convention, maybe my subconscious picked 32 because I am 32 xD

SU2_CFD/src/output/CFlowOutput.cpp Show resolved Hide resolved
SU2_CFD/src/output/CFlowOutput.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/output/COutput.cpp
Comment on lines +2176 to +2178
#ifndef NDEBUG
std::cout << str << std::endl;
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Keeping additional debugging code behind preprocessor directives seems like an concept Su2 could adopt... but if more people use NDEBUG, that might quickly bloat screen output. (so maybe some additional info needs to be printed ahead? idk just leave it like that, i am more thinking to myself)

@pcarruscag @TobiKattmann
Apply suggestions from code review
9b99cc6 
Co-authored-by: TobiKattmann <31306376+TobiKattmann@users.noreply.github.com>
@pcarruscag
Copy link
Member Author

What parts do you find trickier? I can try to improve comments.
I guess the parsing of name : type{expression}[markers] is a bit messy.

@lgtm-com
Copy link

lgtm-com bot commented Jul 29, 2022

This pull request fixes 1 alert when merging 9b99cc6 into eed1e67 - view on LGTM.com

fixed alerts:

  • 1 for Comparison of narrow type with wide type in loop condition

WallyMaier
WallyMaier approved these changes Jul 31, 2022
View reviewed changes
Copy link
Contributor

@WallyMaier WallyMaier left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for adding this! I'll see how quickly I can break it :)

@pcarruscag pcarruscag merged commit 44921de into develop Aug 7, 2022
@pcarruscag pcarruscag deleted the feature_custom_functions branch August 7, 2022 05:10
@Eurus-Way
Copy link

Eurus-Way commented Oct 11, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TobiKattmann TobiKattmann TobiKattmann approved these changes

@WallyMaier WallyMaier WallyMaier approved these changes

Assignees
No one assigned
Labels
changelog:feature PR: merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pcarruscag @TobiKattmann @Eurus-Way @WallyMaier