Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Pointwise boundary surface values #1920
base: main
Are you sure you want to change the base?
Pointwise boundary surface values #1920
Changes from 2 commits
9284b11
0f6ad41
2e79e4e
500518a
eb93c31
9c1cc7a
9e4ad12
0fef7b4
dd426b9
b29b2a9
f010eab
c10c3c6
a281eee
178af8c
e0e7320
a0abcda
1310ade
de99163
3ba6031
bda1a2b
e939485
6f8eb63
9c51d7f
022dc28
8419088
10cfb00
57d2b38
b995e3f
cb826db
f39d7f3
cf2fb11
2bc6001
f8b2473
cd18b83
bdb82f1
5f5f464
db33b64
b9021a4
bac4e4d
29d2663
a7b9ec1
efe8ddf
6c7657e
4180757
ed3abf0
9fd0b58
9785a83
e25e462
f0e7047
012680c
880674e
1b52899
c37b203
e6ad84b
a7b5f8f
842eaeb
0374850
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't attach this to the proper line, but the docstring should include the pointwise analysis in its list of arguments and ideally also (briefly) explain how it can be properly used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not related to particular lines of code, but more a general remark:
Does it really make sense to have a completely separate implementation for pointwise surface data and surface integrals? Wouldn't they naturally share a lot of the implementation, maybe except the integration part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is certainly true. We could try to refactor the current implementation by calling different functions after
since this part is shared among all 4 versions (integrals/points, viscous/inviscid)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be able to call this from one function we would need to add variables to the
AnalysisSurfaceIntegral/Pointwise
structs.For Integral, this would be
surface_integral = zero(eltype(u))
and for the pointwise stuffThen we still have to figure out how to treat the passing in of the iteration number.
Having said this, while we certainly have code-duplication in the boilerplate part, we currently have
analyze
methods with 4 different function signatures.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function (and most others in this file) would benefit from having at least a short, one-line comment on what they are doing or when they are called. For example, below there are two fairly complex
analyze
methods that have very similar arguments and it takes a while to figure out their different use cases.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why define a default at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This default is defined to give a helpful error message in case a user forgets to define
varname
for their new surface quantity. The comment answers the question “Why not set the default name to something trivial like 'variable'?". I am open to doing it differently or removing it.