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

Add look-up table class #1637

Merged
merged 120 commits into from
Aug 5, 2022
Merged

Add look-up table class #1637

merged 120 commits into from
Aug 5, 2022

Conversation

bigfooted
Copy link
Contributor

@bigfooted bigfooted commented May 16, 2022

Proposed Changes

Give a brief overview of your contribution here in a few sentences.
Adds the functionality of reading fluid data from an unstructured triangulated lookup table and interpolating. No coupling with a solver (combustion) yet, at this point we will only add a verification test

Related Work

Resolve any issues (bug fix or feature request), note any related PRs, or mention interactions with the work of others, if any.
We split #1223 into smaller chunks

PR Checklist

Put an X by all that apply. You can fill this out after submitting the PR. If you have any questions, don't hesitate to ask! We want to help. These are a guide for you to know what the reviewers will be looking for in your contribution.

  • 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.

@bigfooted
Copy link
Contributor Author

At this stage, I'd like to add only the functionality of reading and interpolating a lookuptable, without immediately implementing a combustion solver that uses it.
I'd like to add a small verification test, would this be implemented in the MMS?

@pr-triage pr-triage bot removed the PR: draft label May 16, 2022
@lgtm-com
Copy link

lgtm-com bot commented May 16, 2022

This pull request introduces 1 alert when merging 1202a60 into 2ce680a - view on LGTM.com

new alerts:

  • 1 for FIXME comment

@pcarruscag
Copy link
Member

The test would go in UnitTests

bigfooted and others added 2 commits May 17, 2022 14:15
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
@lgtm-com
Copy link

lgtm-com bot commented May 17, 2022

This pull request introduces 1 alert when merging c439501 into 6ca6ec2 - view on LGTM.com

new alerts:

  • 1 for FIXME comment

@lgtm-com
Copy link

lgtm-com bot commented May 18, 2022

This pull request introduces 1 alert when merging 9bb027f into 6ca6ec2 - view on LGTM.com

new alerts:

  • 1 for FIXME comment

@lgtm-com
Copy link

lgtm-com bot commented May 20, 2022

This pull request introduces 1 alert when merging 0b7fdb0 into da4c75d - view on LGTM.com

new alerts:

  • 1 for FIXME comment

@lgtm-com
Copy link

lgtm-com bot commented May 21, 2022

This pull request introduces 1 alert when merging 7cb4919 into f15fe75 - view on LGTM.com

new alerts:

  • 1 for FIXME comment

@lgtm-com
Copy link

lgtm-com bot commented May 24, 2022

This pull request introduces 1 alert when merging 1b00541 into 7a57336 - view on LGTM.com

new alerts:

  • 1 for FIXME comment

@lgtm-com
Copy link

lgtm-com bot commented May 27, 2022

This pull request introduces 1 alert when merging f4186f6 into ead83fe - view on LGTM.com

new alerts:

  • 1 for FIXME comment

SU2_CFD/include/numerics/CFileReaderLUT.hpp Outdated Show resolved Hide resolved
SU2_CFD/include/numerics/CFileReaderLUT.hpp Outdated Show resolved Hide resolved
SU2_CFD/include/numerics/CFileReaderLUT.hpp Outdated Show resolved Hide resolved
SU2_CFD/include/numerics/CLookUpTable.hpp Outdated Show resolved Hide resolved
SU2_CFD/include/numerics/CLookUpTable.hpp Outdated Show resolved Hide resolved
SU2_CFD/src/numerics/CTrapezoidalMap.cpp Outdated Show resolved Hide resolved
Comment on lines 34 to 35
CTrapezoidalMap::CTrapezoidalMap(vector<su2double> const& samples_x, vector<su2double> const& samples_y,
vector<vector<unsigned long> > const& edges,
Copy link
Member

Choose a reason for hiding this comment

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

clang format

Copy link
Contributor Author

Choose a reason for hiding this comment

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

clang-format says this is OK. What would you like this to be? Every argument on a line when the list of arguments is too long? Would you like to see the behavior of:
BinPackParameters:false
If false, a function declaration’s or function definition’s parameters will either all be on the same line or will have one line each.

Copy link
Member

Choose a reason for hiding this comment

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

Having the const first, but if clang format doesn't complain it's fine I guess, even if not super consistent.

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 noticed that BinPackParameters:false does this (put const in front) as well, maybe we should add it to .clang-format?

SU2_CFD/src/numerics/CTrapezoidalMap.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/numerics/CLookUpTable.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/numerics/CLookUpTable.cpp Outdated Show resolved Hide resolved
bigfooted and others added 3 commits May 28, 2022 09:19
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
Copy link
Member

@pcarruscag pcarruscag left a comment

Choose a reason for hiding this comment

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

Ah I think I forgot yesterday, these files would be better in common/*/containers

bigfooted and others added 2 commits May 28, 2022 10:53
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
@pcarruscag pcarruscag changed the title Feature lookuptable Add look-up table class Jul 23, 2022
Common/include/containers/CFileReaderLUT.hpp Show resolved Hide resolved
Common/include/containers/CLookUpTable.hpp Outdated Show resolved Hide resolved
Common/include/containers/CLookUpTable.hpp Outdated Show resolved Hide resolved
Common/include/containers/CTrapezoidalMap.hpp Outdated Show resolved Hide resolved
Common/include/containers/CLookUpTable.hpp Outdated Show resolved Hide resolved
Common/src/containers/CLookUpTable.cpp Outdated Show resolved Hide resolved
Common/src/containers/CTrapezoidalMap.cpp Outdated Show resolved Hide resolved
UnitTests/Common/containers/CLookupTable_tests.cpp Outdated Show resolved Hide resolved
UnitTests/Common/containers/CLookupTable_tests.cpp Outdated Show resolved Hide resolved
UnitTests/Common/containers/lookuptable.drg Show resolved Hide resolved
bigfooted and others added 11 commits July 24, 2022 12:57
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
@bigfooted bigfooted merged commit b01a639 into develop Aug 5, 2022
@bigfooted bigfooted deleted the feature_lookuptable branch August 5, 2022 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants