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

Feature/dlw components too #46

Merged
merged 16 commits into from
Mar 17, 2023
Merged

Feature/dlw components too #46

merged 16 commits into from
Mar 17, 2023

Conversation

whaeck
Copy link
Member

@whaeck whaeck commented Oct 26, 2021

Added components from the DLW block:

  • outgoing energy distributions (ACE LAW=4)
  • Kalbach-Mann systematics (ACE LAW=44)

As usual, some stuff comes first ;-)

@whaeck whaeck requested a review from nathangibson14 October 26, 2021 17:10
@whaeck whaeck requested a review from cjosey March 8, 2023 16:17
Copy link

@cjosey cjosey left a comment

Choose a reason for hiding this comment

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

Alright. So, there are a few notes here.

Terms marked [clang-tidy] mean that that tool told me about these. If they were low priority, I noted the first time they came up only so that you can tell me if you want to persist in future reviews with these checks. The recommended ones I noted down every time they came up.

There were two concerning places. First, there was using the cosines in the Python to set the energy values. The second was using an array after it had been moved. Addressing those (or telling me I'm wrong) are what block approval.

The rest is suggestions and entirely up to you.


// local includes
#include "ACEtk/block/KalbachMannDistributionData.hpp"
#include "views.hpp"
Copy link

Choose a reason for hiding this comment

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

[clang-tidy - low priority]
Suggest sorting includes alphabetically.

(I note this is consistent with most other implementations - will only mention here)

namespace block {

void wrapKalbachMannDistributionData( python::module& module,
python::module& ) {
Copy link

Choose a reason for hiding this comment

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

[clang-tidy - recommended]
Add /* unused */ or equivalent for unused arguments.

(I note this is consistent with most other implementations - will only mention here)

Copy link
Member Author

Choose a reason for hiding this comment

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

but later

python/test/CMakeLists.txt Show resolved Hide resolved
whaeck added a commit that referenced this pull request Mar 13, 2023
@whaeck
Copy link
Member Author

whaeck commented Mar 13, 2023

@cjosey fix/review-1 has been pushed and contains the changes mentioned above. In addition, I checked every generateXSS function for all blocks and made const/const auto/static_cast changes as required. Seems my code is just plain sloppy :-)

@cjosey
Copy link

cjosey commented Mar 17, 2023

All components have been adequately addressed in discussions. Here is the final .clang-tidy from this PR:

# Rationale for specific removals:
# Specific project removals:
#   We are not those projects, and these checks often have requirements
#   we do not care about.
# -llvm-header-guard
#   I like this, but the check seems to get confused by CMake and
#   tries to do the path all the way to root. I've yet to figure out why.
# -modernize-use-trailing-return-type
#   I've never seen a solid argument for the "full auto" format. It feels
#   like verbosity for the sake of verbosity
# -llvm-include-order
#   Does not match coding style
# -modernize-concat-nested-namespaces
#   Does not match coding style
# The following are of interest, but will not be added now.
# Namespace/namespace comments
# -readability-named-parameter
# -modernize-use-nodiscard
Checks: '*,-llvmlibc-*,-altera-*,-fuchsia-*,-hicpp-*,-llvm-namespace-comment,-google-readability-namespace-comments,-llvm-header-guard,-modernize-use-trailing-return-type,-llvm-include-order,-readability-named-parameter,-modernize-use-nodiscard,-modernize-concat-nested-namespaces'
# Need to use a complicated match as src/ matches [dependency]-src/
# and folders in Hana.
HeaderFilterRegex: 'src/ACEtk|src/utilities'

When we get near the end of this project, we can do a final pass with this. Future reviews will be done with this configuration.

@whaeck whaeck merged commit 786371d into develop Mar 17, 2023
@whaeck whaeck deleted the feature/dlw-components-too branch March 17, 2023 18:09
whaeck added a commit that referenced this pull request Jun 26, 2023
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