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

Add isFci for fields #2887

Open
wants to merge 12 commits into
base: next
Choose a base branch
from
Open

Add isFci for fields #2887

wants to merge 12 commits into from

Conversation

dschwoerer
Copy link
Contributor

No description provided.

dschwoerer and others added 3 commits March 19, 2024 15:56
@dschwoerer
Add isFci to check if a field is a FCI field.
fa357c1
@dschwoerer
Improve isFci
ca59edb 
Ensure this does not crash if coordinates or transform is not set.
In this case no FCI transformation is set, and this returns false.
@dschwoerer @github-actions
Apply clang-format changes
8b1fbba
ZedThree
ZedThree reviewed Mar 19, 2024
View reviewed changes
include/bout/field.hxx Outdated
@@ -178,6 +178,18 @@ inline bool areFieldsCompatible(const Field& field1, const Field& field2) {
#define ASSERT1_FIELDS_COMPATIBLE(field1, field2) ;
#endif

template <typename F>
inline bool isFci(const F& f) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe canBeFieldAligned? Not sure.

Also, should this be a member function instead? Then it wouldn't need to be templated. If it should really be a free function, it would be good to add a static_assert (see emptyFrom etc) to give a nicer error message if passed the wrong type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe canBeFieldAligned? Not sure.

But I want to know whether it is FCI. If we ever have a different, non-field aligned, non FCI method, we might need to reconsider, but I think isFci (or isFCI) is better, as you can use BOUT++ also for non magnetic simulations, and that is also not field aligned, but not FCI either.

Also, should this be a member function instead? Then it wouldn't need to be templated. If it should really be a free function, it would be good to add a static_assert (see emptyFrom etc) to give a nicer error message if passed the wrong type.

Good point, will make it a member function 👍

@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

github-actions[bot]
github-actions bot reviewed Mar 20, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/field/field.cxx Outdated Show resolved Hide resolved
@dschwoerer dschwoerer added small-change Changes less than 100 lines - should be quick to review FCI labels Apr 12, 2024
@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@dschwoerer dschwoerer mentioned this pull request Oct 22, 2024
Merged
@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

github-actions[bot]
github-actions bot reviewed Oct 28, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

include/bout/mesh.hxx
ASSERT1(location != CELL_DEFAULT);
ASSERT1(location != CELL_VSHIFT);

auto found = coords_map.find(location);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: method 'getAllowedStaggerLoc' can be made static [readability-convert-member-functions-to-static]

Suggested change
auto found = coords_map.find(location);
static CELL_LOC getAllowedStaggerLoc(DIRECTION direction) {

include/bout/mesh.hxx
return false;
}
return not coords->getParallelTransform().canToFromFieldAligned();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: parameter 'location' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]

Suggested change
}
createDefaultCoordinates(CELL_LOC location,

@dschwoerer
Copy link
Contributor Author

@ZedThree clang-tidy-review is failing:

Saving metadata
Traceback (most recent call last):
  File "/usr/local/bin/review", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/usr/local/lib/python3.12/dist-packages/clang_tidy_review/review.py", line 174, in main
    post_annotations(pull_request, review)
  File "/usr/local/lib/python3.12/dist-packages/clang_tidy_review/__init__.py", line 1199, in post_annotations
    pull_request.post_annotations(body)
  File "/usr/local/lib/python3.12/dist-packages/clang_tidy_review/__init__.py", line 353, in post_annotations
    self.repo._requester.requestJsonAndCheck(
  File "/usr/local/lib/python3.12/dist-packages/github/Requester.py", line 550, in requestJsonAndCheck
    return self.__check(*self.requestJson(verb, url, parameters, headers, input, self.__customConnection(url)))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/dist-packages/github/Requester.py", line 611, in __check
    raise self.createException(status, responseHeaders, data)
github.GithubException.GithubException: 422 {"message": "Invalid request.\n\nFor 'links/0/schema', nil is not an object.", "documentation_url": "https://docs.github.com/rest/checks/runs#create-a-check-run", "status": "422"}

Is this maybe due to some of your recent changes?

@ZedThree
Copy link
Member

ZedThree commented Nov 6, 2024

@dschwoerer Yes, but I don't understand what exactly. I need to investigate further

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

@github-actions github-actions[bot] github-actions[bot] left review comments

@ZedThree ZedThree ZedThree left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
FCI small-change Changes less than 100 lines - should be quick to review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dschwoerer @ZedThree @bendudson