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 code-formatting with black for python and with clang-format for c++ #1630

Merged
merged 9 commits into from
Mar 13, 2023

Conversation

hhorii
Copy link
Collaborator

@hhorii hhorii commented Oct 25, 2022

Summary

Adds code-formatting with black and pre-commit

Details and comments

Aer has not used code-formatting. This PR introduces black for all python files and clang-format for all cpp files.
tox -elint validates lint and tox -eblack, and tox -epre-commit format python and c++ codes respectively.

@hhorii hhorii added the stable-backport-potential The issue or PR might be minimal and/or import enough to backport to stable label Oct 25, 2022
@hhorii
Copy link
Collaborator Author

hhorii commented Oct 25, 2022

I hope to apply this to 0.11 stable. There are few changes in cpp since 0.11 and this PR includes many changes in cpp.

@mtreinish
Copy link
Member

We should not apply this to the stable branch, it's a risky change with no end user benefit the exact kind of thing we should not backport.

@hhorii hhorii removed the stable-backport-potential The issue or PR might be minimal and/or import enough to backport to stable label Oct 25, 2022
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

In general I'm definitely in favor of making a change like this to use auto-formatting for code. We should also do the same for using black on the python code. I do have some questions, mainly around the use of precommit in particular and the creation of a separate CI job to check it.

My concern with pre-commit is it's yet another tool to manage running tests and linting. I'm just wondering what in particular it's providing here. Like if we just called clang-format in our tox config would that accomplish the same thing, or is pre-commit here doing some extra setup like installing dependencies that tox won't provide us? Like what happens if you runpre-commit without clang installed? In general I think it would be good to try and bake this style correction into our already existing workflows for this (assuming it's feasible).

For the CI job, I think we should bake running the clang-format job into our existing lint job. We're already running that job to validate python code formatting so we can just add C++ there too (we do something similar in other projects with cargo fmt for rust).

The other thought I had is around stable configuration, does clang formatting change between releases? Would it make sense to define a static config file for clang format to use for this.

Also, we should update CONTRIBUTING.md to mention how to run clang format as it will be a custom lint step for aer code.

As for the git blame we can add a .git-blame-ignore-revs file to the repo and document it, like what we have in terra from when we adopted black: https://github.com/Qiskit/qiskit-terra/blob/main/.git-blame-ignore-revs and https://github.com/Qiskit/qiskit-terra/blob/main/CONTRIBUTING.md#dealing-with-the-git-blame-ignore-list

@hhorii
Copy link
Collaborator Author

hhorii commented Oct 26, 2022

Currently, Aer uses pycodestyle and pylint for format-checking of python codes. They can not cover cpp codes in my understanding. pre-commit has hooks for various languages and we will be able to use it for the all. Also I confirmed that pre-commit does not need LLVM installation.
I reused tests.yml for pre-commit instead of standalone workflow.

@jakelishman
Copy link
Member

fwiw: the clang-format recipe for pre-commit uses the PyPI package of the same name, which basically just stuffs the clang-format executable into a Python wheel for each system. That's a pretty questionable mismatch of different package managers to me, but as long as pre-commit isolates its internal Python environments (which it does seem to do) to avoid polluting my PATH with its clang-format, that's not the end of the world for me.

If we're bikeshedding the use of pre-commit at all, then I would be ok with putting fast formatting fixers in such a file (total runtime < 1s), but against putting linters in it. I don't think commiting should be a heavy operation, and linting (even when notionally limited to changed files - linters usually need to explore more of the package statically anyway) is not sufficiently fast. The sort of bugs it catches are often fine to handle in CI anyway - I don't think it buys anything to force devs to run a not-exactly-the-same-as-CI lint locally on every commit.

I also don't entirely see the point in allowing pre-commit hook that might cause a commit to be rejected (like lint). We're going to have to run all the proper formatting/lint suites in CI anyway because we can't enforce that somebody installs the pre-commit hooks (or doesn't use git commit --no-verify), so mostly we're just making certain things harder, like storing debug progress in series of commits to be rebased, for the users who might do that sort of thing, and the users who won't are the ones most likely to not have pre-commit configured anwyay.

(That said, even if we did put the config in the repo, I probably wouldn't actually install the hooks myself - I usually just configure my editor to auto-format on write / manual command.)

@hhorii
Copy link
Collaborator Author

hhorii commented Oct 27, 2022

I also don't entirely see the point in allowing pre-commit hook that might cause a commit to be rejected (like lint).

@mtreinish If e8ee5f9 adds pre-commit with existing Tests/lint workflow and commit will be failed if format is wrong. Original 8ee44e1 uses a different workflow for pre-commit and follows @jakelishman wants.

@jakelishman
Copy link
Member

I mean, I'm not really in favour of another tool either. In this case, we could just skip the middle man and add the clang-format PyPI package to tox and just continue to use that.

I don't feel super strongly about it, and I wouldn't hold anything up though, especially since I don't do much development on Aer.

@hhorii hhorii changed the title add precommit and force code-formatting add code-formatting with black for python and with clang-format for c++ Oct 28, 2022
@hhorii
Copy link
Collaborator Author

hhorii commented Nov 2, 2022

We will introduce new code-formatters as the last commit for 0.12.0 release.

@hhorii hhorii added this to the Aer 0.12.0 milestone Jan 30, 2023
@hhorii hhorii changed the title add code-formatting with black for python and with clang-format for c++ [WIP] add code-formatting with black for python and with clang-format for c++ Feb 20, 2023
@hhorii hhorii force-pushed the add_precommit branch 7 times, most recently from a842415 to e7e091e Compare February 20, 2023 13:34
@hhorii hhorii force-pushed the add_precommit branch 4 times, most recently from 79907f5 to 422f63e Compare March 2, 2023 14:31
@hhorii
Copy link
Collaborator Author

hhorii commented Mar 2, 2023

Compilation for standalone in windows-2019 fails after applying clang-format. Now I'm narrowing scope of formatting to identify root causes with CI runs.

@hhorii hhorii changed the title [WIP] add code-formatting with black for python and with clang-format for c++ add code-formatting with black for python and with clang-format for c++ Mar 10, 2023
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

Thanks for doing this, I'm looking forward to having consistent code formatting everywhere.

I'm going to run this locally on a fresh clone of the repo to try and verify the giant diff is just the application of clang-format and black to all the source code prior to approving. But before I start working on that I left a few inline comments.

CONTRIBUTING.md Outdated
tool that will automatically update the code formatting to a consistent style.
The second tool is [pylint](https://www.pylint.org/) which is a code linter
which does a deeper analysis of the Python code to find both style issues and
potential bugs and other common issues in Python.
Copy link
Member

Choose a reason for hiding this comment

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

There are only 2 tools mentioned here, you forgot to mention clang format here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was written in an old commit, but it was wrongly overwritten while resolving conflicts...

@@ -5,7 +5,8 @@ scikit-build>=0.11.0
asv
cvxpy>=1.0.0,<1.1.15;platform_system != 'Windows' and python_version < '3.11'
pylint
pycodestyle
black[jupyter]~=22.0
Copy link
Member

Choose a reason for hiding this comment

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

While we're doing this for the first time to do you want to use ~=23.0 for the latest version? black does a compatibility for a year's worth of releases and then integrates behavior/format changes with a new major version for each year. Since we're in 2023 now we can use the newer version here. Not that it particularly matters, but I figured it would just save us from having to do an upgrade and big format change again at some point in the future.

tox.ini Outdated
commands =
pycodestyle --ignore=E402,W503,W504 --max-line-length=100 qiskit_aer
sh tools/clang-format.sh --Werror -n
black --line-length 100 --check {posargs} qiskit_aer test tools setup.py
Copy link
Member

Choose a reason for hiding this comment

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

The --line-length argument here is causing this check to diverge with what you have committed to your branch. Can you add the configuration to the pyproject.toml file like: https://github.com/Qiskit/qiskit-terra/blob/main/pyproject.toml#L5-L7 and remove the argument here? That way we'll have the same line length consistently between all uses of the black command.

tox.ini Outdated
Comment on lines 48 to 53
[pycodestyle]
max-line-length = 105
# default ignores + E741 because of opflow global variable I
# + E203 because of a difference of opinion with black
# codebase does currently comply with: E133, E242, E704, W505
ignore = E121, E123, E126, E133, E226, E241, E242, E704, W503, W504, W505, E741, E203
Copy link
Member

Choose a reason for hiding this comment

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

Is this used by anything?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no longer :-)

#!/bin/bash

cd `dirname $0`/..
IGNORE_FILES="src/misc/warnings.hpp"
Copy link
Member

Choose a reason for hiding this comment

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

Why is this ignored?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

clang-format wrongly formats pragma.

   DISABLE_WARNING(-Wall)                                                       \
   DISABLE_WARNING(-Wextra)                                                     \
   DISABLE_WARNING(-Wshadow)                                                    \
-  DISABLE_WARNING(-Wfloat-equal)                                               \
+  DISABLE_WARNING(-Wfloat - equal)                                             \
   DISABLE_WARNING(-Wundef)                                                     \
   DISABLE_WARNING(-Wpedantic)                                                  \
-  DISABLE_WARNING(-Wredundant-decls)                                           \
+  DISABLE_WARNING(-Wredundant - decls)                                         \
   DISABLE_WARNING(-Wsign-compare)

I tried to use WhitespaceSensitiveMacros but not yet find a good configuration that disable formatting of white spaces in macro.

IGNORE_FILES="src/misc/warnings.hpp"

ERROR=0
for FILE in `find src -iname "*.hpp" -or -iname "*.cpp"`; do
Copy link
Member

Choose a reason for hiding this comment

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

What about the c++ files in qiskit_aer and test?

Copy link
Collaborator Author

@hhorii hhorii Mar 10, 2023

Choose a reason for hiding this comment

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

good comment. let me add them into the list. maybe contrib should be included, too.

CONTRIBUTING.md Outdated Show resolved Hide resolved
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

I caught one small oversight in the CI config. Also the black config isn't present in the pyproject.toml so it has the default line set of 88 characters instead of 100 (which I assume you were going for).

Besides that I'm having trouble reproducing the same output on a local clone (I'm getting subtle differences between my test repo and your branch). I'm not sure what's going on for the most part the diff is identical except for:

 controllers/aer_controller.hpp                                  |    2 +-
 framework/config.hpp                                            |    1 +
 framework/pybind_casts.hpp                                      |    1 +
 open_pulse/eval_hamiltonian.hpp                                 |   10 ++++++----
 simulators/matrix_product_state/matrix_product_state.hpp        |   10 +++++-----
 simulators/matrix_product_state/matrix_product_state_tensor.hpp |   21 +++++++++++----------
 simulators/statevector/chunk/cuStateVec_chunk_container.hpp     |    6 +++---
 simulators/statevector/statevector_state.hpp                    |   10 +++++-----
 simulators/tensor_network/tensor_net.hpp                        |    2 +-
 simulators/tensor_network/tensor_net_state.hpp                  |   10 +++++-----
 10 files changed, 39 insertions(+), 34 deletions(-)

I'm wondering if your local branch wasn't clean when you committed things. Like there are a few places the diff is more than formatting like extra #include statements which is more than I'd expect clang format to do.

.github/workflows/tests.yml Outdated Show resolved Hide resolved
@mtreinish
Copy link
Member

This is the full diff the a side is my fresh clone that I copied the config files into and ran tox -eclang-format and tox -eblack on. The b side is your PR branch at the current HEAD (54399d1). Some of this diff looks like things I'd expect clang format to fix like comment spacing and comment types. But others I'm confused by. I might have missed a config in my copy over or maybe there is a clang format difference on linux?

diff --git a/src/controllers/aer_controller.hpp b/home/mtreinish/git/qiskit/qiskit-aer/src/controllers/aer_controller.hpp
index bf144fc2..f0ee3acd 100644
--- a/src/controllers/aer_controller.hpp
+++ b/home/mtreinish/git/qiskit/qiskit-aer/src/controllers/aer_controller.hpp
@@ -989,7 +989,7 @@ Result Controller::execute(std::vector<Circuit> &circuits,
     const int NUM_RESULTS = result.results.size();
     // following looks very similar but we have to separate them to avoid omp
     // nested loops that causes performance degradation (DO NOT use if statement
-    //in #pragma omp)
+    // in #pragma omp)
     if (parallel_experiments_ == 1) {
       for (int j = 0; j < NUM_RESULTS; ++j) {
         set_parallelization_circuit(circuits[j], noise_model, methods[j]);
diff --git a/src/framework/config.hpp b/home/mtreinish/git/qiskit/qiskit-aer/src/framework/config.hpp
index 6ebe7865..2890747a 100644
--- a/src/framework/config.hpp
+++ b/home/mtreinish/git/qiskit/qiskit-aer/src/framework/config.hpp
@@ -15,6 +15,7 @@
 #ifndef _aer_framework_config_hpp_
 #define _aer_framework_config_hpp_
 
+#include "json.hpp"
 #include "types.hpp"
 #include <optional>
 #include <string>
diff --git a/src/framework/pybind_casts.hpp b/home/mtreinish/git/qiskit/qiskit-aer/src/framework/pybind_casts.hpp
index 1db098fb..7e18ee30 100644
--- a/src/framework/pybind_casts.hpp
+++ b/home/mtreinish/git/qiskit/qiskit-aer/src/framework/pybind_casts.hpp
@@ -16,6 +16,7 @@
 #define _aer_framework_pybind_casts_hpp_
 
 #include "../simulators/stabilizer/clifford.hpp"
+#include <pybind11/numpy.h>
 
 namespace py = pybind11;
 
diff --git a/src/open_pulse/eval_hamiltonian.hpp b/home/mtreinish/git/qiskit/qiskit-aer/src/open_pulse/eval_hamiltonian.hpp
index aea06967..dfa2197e 100644
--- a/src/open_pulse/eval_hamiltonian.hpp
+++ b/home/mtreinish/git/qiskit/qiskit-aer/src/open_pulse/eval_hamiltonian.hpp
@@ -15,6 +15,8 @@
 #ifndef _EVAL_HAMILTONIAN_HPP
 #define _EVAL_HAMILTONIAN_HPP
 
+#include "framework/types.hpp"
+#include "iterators.hpp"
 #include "misc/warnings.hpp"
 #include <complex>
 #include <string>
@@ -42,10 +44,10 @@ struct hash<ParserValues> {
 } // namespace std
 
 // TODO: Document
-complex_t evaluate_hamiltonian_expression(
+AER::complex_t evaluate_hamiltonian_expression(
     const std::string &expr_string, const std::vector<double> &vars,
     const std::vector<std::string> &vars_names,
-    const std::unordered_map<std::string, complex_t> &chan_values) {
+    const std::unordered_map<std::string, AER::complex_t> &chan_values) {
 
   static std::unordered_map<std::string, std::unique_ptr<ParserValues>>
       parser_expr;
@@ -75,7 +77,7 @@ complex_t evaluate_hamiltonian_expression(
   // std::cout << "Getting parser " << std::hex << parser << "\n";
 
   auto maybe_update_value = [parser](const std::string &var_name,
-                                     const complex_t &var_value) {
+                                     const AER::complex_t &var_value) {
     if (parser->var_values.find(var_name) == parser->var_values.end()) {
       parser->var_values.emplace(var_name,
                                  std::make_unique<mup::Value>(var_value));
@@ -93,7 +95,7 @@ complex_t evaluate_hamiltonian_expression(
 
   for (const auto &idx_var : enumerate(vars)) {
     size_t index = idx_var.first;
-    auto var_value = static_cast<complex_t>(idx_var.second);
+    auto var_value = static_cast<AER::complex_t>(idx_var.second);
     maybe_update_value(vars_names[index], var_value);
   }
 
diff --git a/src/simulators/matrix_product_state/matrix_product_state.hpp b/home/mtreinish/git/qiskit/qiskit-aer/src/simulators/matrix_product_state/matrix_product_state.hpp
index 354e6021..4105fd3b 100644
--- a/src/simulators/matrix_product_state/matrix_product_state.hpp
+++ b/home/mtreinish/git/qiskit/qiskit-aer/src/simulators/matrix_product_state/matrix_product_state.hpp
@@ -274,14 +274,14 @@ const stringmap_t<Gates>
                      {"rx", Gates::rx},     // Pauli-X rotation gate
                      {"ry", Gates::ry},     // Pauli-Y rotation gate
                      {"rz", Gates::rz},     // Pauli-Z rotation gate
-                     // Waltz Gates
+                     /* Waltz Gates */
                      {"p", Gates::u1},  // zero-X90 pulse waltz gate
                      {"u1", Gates::u1}, // zero-X90 pulse waltz gate
                      {"u2", Gates::u2}, // single-X90 pulse waltz gate
                      {"u3", Gates::u3}, // two X90 pulse waltz gate
                      {"u", Gates::u3},  // two X90 pulse waltz gate
                      {"U", Gates::u3},  // two X90 pulse waltz gate
-                     // Two-qubit gates
+                     /* Two-qubit gates */
                      {"CX", Gates::cx},   // Controlled-X gate (CNOT)
                      {"cx", Gates::cx},   // Controlled-X gate (CNOT)
                      {"cy", Gates::cy},   // Controlled-Y gate
@@ -294,10 +294,10 @@ const stringmap_t<Gates>
                      {"ryy", Gates::ryy},   // Pauli-YY rotation gate
                      {"rzz", Gates::rzz},   // Pauli-ZZ rotation gate
                      {"rzx", Gates::rzx},   // Pauli-ZX rotation gate
-                     // Three-qubit gates
+                     /* Three-qubit gates */
                      {"ccx", Gates::ccx}, // Controlled-CX gate (Toffoli)
                      {"cswap", Gates::cswap},
-                     // Pauli
+                     /* Pauli */
                      {"pauli", Gates::pauli}});
 
 //=========================================================================
@@ -813,4 +813,4 @@ std::pair<uint_t, double> State::sample_measure_with_prob(const reg_t &qubits,
 //-------------------------------------------------------------------------
 } // end namespace AER
 //-------------------------------------------------------------------------
-#endif
+#endif
\ No newline at end of file
diff --git a/src/simulators/matrix_product_state/matrix_product_state_tensor.hpp b/home/mtreinish/git/qiskit/qiskit-aer/src/simulators/matrix_product_state/matrix_product_state_tensor.hpp
index f09dbe53..01559258 100644
--- a/src/simulators/matrix_product_state/matrix_product_state_tensor.hpp
+++ b/home/mtreinish/git/qiskit/qiskit-aer/src/simulators/matrix_product_state/matrix_product_state_tensor.hpp
@@ -556,9 +556,11 @@ void MPS_Tensor::contract_2_dimensions(const MPS_Tensor &left_gamma,
                                          (omp_threads > 1))                    \
     num_threads(omp_threads)
 #endif
-  for (int_t l_row = 0; l_row < left_rows; l_row++)
-    for (int_t r_col = 0; r_col < right_columns; r_col++)
+  for (int_t l_row = 0; l_row < left_rows; l_row++) {
+    for (int_t r_col = 0; r_col < right_columns; r_col++) {
       result(l_row, r_col) = 0;
+    }
+  }
 
 #ifdef _WIN32
 #pragma omp parallel for if ((omp_limit > MATRIX_OMP_THRESHOLD) &&             \
@@ -568,24 +570,23 @@ void MPS_Tensor::contract_2_dimensions(const MPS_Tensor &left_gamma,
                                          (omp_threads > 1))                    \
     num_threads(omp_threads)
 #endif
-  for (int_t l_row = 0; l_row < left_rows; l_row++)
+  for (int_t l_row = 0; l_row < left_rows; l_row++) {
     for (int_t r_col = 0; r_col < right_columns; r_col++) {
-
       for (int_t size = 0; size < left_size; size++)
         for (int_t index = 0; index < left_columns; index++) {
           result(l_row, r_col) += left_gamma.data_[size](l_row, index) *
                                   right_gamma.data_[size](index, r_col);
         }
     }
+  }
 }
 //---------------------------------------------------------------
 // function name: Decompose
-// Description: Decompose a tensor into two Gamma tensors and the Lambda between
-// 				them. Usually used after applying a 2-qubit
-// gate. Parameters: MPS_Tensor &temp - the tensor to decompose.
-//			   MPS_Tensor &left_gamma, &right_gamma , rvector_t &lambda
-//-
-// 			   tensors for the result.
+// Description: Decompose a tensor into two Gamma tensors and the
+//        Lambda between them. Usually used after applying a 2-qubit gate.
+// Parameters: MPS_Tensor &temp - the tensor to decompose.
+//			       MPS_Tensor &left_gamma, &right_gamma
+//             rvector_t &lambda - tensors for the result.
 // Returns: none.
 //---------------------------------------------------------------
 double MPS_Tensor::Decompose(MPS_Tensor &temp, MPS_Tensor &left_gamma,
diff --git a/src/simulators/statevector/chunk/cuStateVec_chunk_container.hpp b/home/mtreinish/git/qiskit/qiskit-aer/src/simulators/statevector/chunk/cuStateVec_chunk_container.hpp
index 88909884..3fd95b94 100644
--- a/src/simulators/statevector/chunk/cuStateVec_chunk_container.hpp
+++ b/home/mtreinish/git/qiskit/qiskit-aer/src/simulators/statevector/chunk/cuStateVec_chunk_container.hpp
@@ -29,11 +29,11 @@ namespace Chunk {
 template <typename data_t>
 class cuStateVecChunkContainer : public DeviceChunkContainer<data_t> {
 protected:
-  custatevecHandle_t
-      custatevec_handle_; // cuStatevec handle for this chunk container
+  custatevecHandle_t custatevec_handle_; // cuStatevec handle
+                                         // for this chunk container
   uint_t custatevec_chunk_total_qubits_; // total qubits of statevector passed
                                          // to ApplyMatrix
-  uint_t custatevec_chunk_count_; // number of counts for all chunks
+  uint_t custatevec_chunk_count_;        // number of counts for all chunks
 
   custatevecDeviceMemHandler_t custatevec_mem_handler_;
   cudaMemPool_t memory_pool_;
diff --git a/src/simulators/statevector/statevector_state.hpp b/home/mtreinish/git/qiskit/qiskit-aer/src/simulators/statevector/statevector_state.hpp
index 8b9ff80a..405c1423 100644
--- a/src/simulators/statevector/statevector_state.hpp
+++ b/home/mtreinish/git/qiskit/qiskit-aer/src/simulators/statevector/statevector_state.hpp
@@ -347,18 +347,18 @@ const stringmap_t<Gates> State<statevec_t>::gateset_(
      {"p", Gates::mcp},       // Parameterized phase gate
      {"sx", Gates::mcsx},     // Sqrt(X) gate
      {"sxdg", Gates::mcsxdg}, // Inverse Sqrt(X) gate
-     // 1-qubit rotation Gates
+     /* 1-qubit rotation Gates */
      {"r", Gates::mcr},   // R rotation gate
      {"rx", Gates::mcrx}, // Pauli-X rotation gate
      {"ry", Gates::mcry}, // Pauli-Y rotation gate
      {"rz", Gates::mcrz}, // Pauli-Z rotation gate
-     // Waltz Gates
+     /* Waltz Gates */
      {"u1", Gates::mcp},  // zero-X90 pulse waltz gate
      {"u2", Gates::mcu2}, // single-X90 pulse waltz gate
      {"u3", Gates::mcu3}, // two X90 pulse waltz gate
      {"u", Gates::mcu3},  // two X90 pulse waltz gate
      {"U", Gates::mcu3},  // two X90 pulse waltz gate
-     // 2-qubit gates
+     /* 2-qubit gates */
      {"CX", Gates::mcx},       // Controlled-X gate (CNOT)
      {"cx", Gates::mcx},       // Controlled-X gate (CNOT)
      {"cy", Gates::mcy},       // Controlled-Y gate
@@ -377,10 +377,10 @@ const stringmap_t<Gates> State<statevec_t>::gateset_(
      {"csx", Gates::mcsx},     // Controlled-Sqrt(X) gate
      {"csxdg", Gates::mcsxdg}, // Controlled-Sqrt(X)dg gate
      {"ecr", Gates::ecr},      // ECR Gate
-     // 3-qubit gates
+     /* 3-qubit gates */
      {"ccx", Gates::mcx},      // Controlled-CX gate (Toffoli)
      {"cswap", Gates::mcswap}, // Controlled SWAP gate (Fredkin)
-     // Multi-qubit controlled gates
+     /* Multi-qubit controlled gates */
      {"mcx", Gates::mcx},       // Multi-controlled-X gate
      {"mcy", Gates::mcy},       // Multi-controlled-Y gate
      {"mcz", Gates::mcz},       // Multi-controlled-Z gate
diff --git a/src/simulators/tensor_network/tensor_net.hpp b/home/mtreinish/git/qiskit/qiskit-aer/src/simulators/tensor_network/tensor_net.hpp
index 3f2cf4e6..f4f94343 100644
--- a/src/simulators/tensor_network/tensor_net.hpp
+++ b/home/mtreinish/git/qiskit/qiskit-aer/src/simulators/tensor_network/tensor_net.hpp
@@ -62,7 +62,7 @@ protected:
   int32_t mode_index_;                                   // index of modes
   std::vector<std::shared_ptr<Tensor<data_t>>> tensors_; // list of tensors
   std::vector<std::shared_ptr<Tensor<data_t>>> qubits_;  // tail tensor for
-                                                        // qubits
+                                                         // qubits
   std::vector<std::shared_ptr<Tensor<data_t>>>
       qubits_sp_;                        // tail tensor for super qubits
   std::vector<int32_t> modes_qubits_;    // tail mode index for qubits
diff --git a/src/simulators/tensor_network/tensor_net_state.hpp b/home/mtreinish/git/qiskit/qiskit-aer/src/simulators/tensor_network/tensor_net_state.hpp
index 72bcd2ef..44a6221c 100644
--- a/src/simulators/tensor_network/tensor_net_state.hpp
+++ b/home/mtreinish/git/qiskit/qiskit-aer/src/simulators/tensor_network/tensor_net_state.hpp
@@ -330,18 +330,18 @@ const stringmap_t<Gates> State<tensor_net_t>::gateset_(
      {"p", Gates::mcp},       // Parameterized phase gate
      {"sx", Gates::mcsx},     // Sqrt(X) gate
      {"sxdg", Gates::mcsxdg}, // Inverse Sqrt(X) gate
-     // 1-qubit rotation Gates
+     /* 1-qubit rotation Gates */
      {"r", Gates::mcr},   // R rotation gate
      {"rx", Gates::mcrx}, // Pauli-X rotation gate
      {"ry", Gates::mcry}, // Pauli-Y rotation gate
      {"rz", Gates::mcrz}, // Pauli-Z rotation gate
-     // Waltz Gates
+     /* Waltz Gates */
      {"u1", Gates::mcp},  // zero-X90 pulse waltz gate
      {"u2", Gates::mcu2}, // single-X90 pulse waltz gate
      {"u3", Gates::mcu3}, // two X90 pulse waltz gate
      {"u", Gates::mcu3},  // two X90 pulse waltz gate
      {"U", Gates::mcu3},  // two X90 pulse waltz gate
-     // 2-qubit gates
+     /* 2-qubit gates */
      {"CX", Gates::mcx},       // Controlled-X gate (CNOT)
      {"cx", Gates::mcx},       // Controlled-X gate (CNOT)
      {"cy", Gates::mcy},       // Controlled-Y gate
@@ -360,10 +360,10 @@ const stringmap_t<Gates> State<tensor_net_t>::gateset_(
      {"csx", Gates::mcsx},     // Controlled-Sqrt(X) gate
      {"csxdg", Gates::mcsxdg}, // Controlled-Sqrt(X)dg gate
      {"ecr", Gates::ecr},      // ECR Gate
-     // 3-qubit gates
+     /* 3-qubit gates */
      {"ccx", Gates::mcx},      // Controlled-CX gate (Toffoli)
      {"cswap", Gates::mcswap}, // Controlled SWAP gate (Fredkin)
-     // Multi-qubit controlled gates
+     /* Multi-qubit controlled gates */
      {"mcx", Gates::mcx},       // Multi-controlled-X gate
      {"mcy", Gates::mcy},       // Multi-controlled-Y gate
      {"mcz", Gates::mcz},       // Multi-controlled-Z gate

@hhorii
Copy link
Collaborator Author

hhorii commented Mar 11, 2023

@mtreinish thank you for your checking.

clang-format -i sometimes generates codes that will not be passed with clang-format -Werror -n. For example,

      {"cswap", Gates::mcswap}, // Controlled SWAP gate (Fredkin)
      // Multi-qubit controlled gates
      {"mcx", Gates::mcx},       // Multi-controlled-X gate

will warns as

src/simulators/tensor_network/tensor_net_state.hpp:365:65: error: code should be clang-formatted [-Wclang-format-violations]
     {"cswap", Gates::mcswap}, // Controlled SWAP gate (Fredkin)

. Therefore, I manually modified the codes after applying clang-format as

      {"cswap", Gates::mcswap}, // Controlled SWAP gate (Fredkin)
      /* Multi-qubit controlled gates */
      {"mcx", Gates::mcx},       // Multi-controlled-X gate

Another disruptive change is order of include files. clang-format sorts include statements and sometimes Aer headers are not self-contained (unfortunately). Then I added some include files or some namespace (such as complex_t -> AER::complex_t).

This diff accidentally comes from my old formatting trials (no RemoveBracesLLVM).

-  for (int_t l_row = 0; l_row < left_rows; l_row++)
-    for (int_t r_col = 0; r_col < right_columns; r_col++)
+  for (int_t l_row = 0; l_row < left_rows; l_row++) {
+    for (int_t r_col = 0; r_col < right_columns; r_col++) {
       result(l_row, r_col) = 0;
+    }
+  }

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

Ok, then given those caveats as things you manually edited I think we can go ahead and merge this. I'm mildly concerned about the fragility of clang format if it's auto-formatting isn't always compatible with it's warning check. But we can update our usage as things progress if we encounter issues.

@mtreinish mtreinish added the automerge This PR will automatically merge once its CI has passed label Mar 13, 2023
@mergify mergify bot merged commit 0366c51 into Qiskit:main Mar 13, 2023
mtreinish added a commit to mtreinish/qiskit-aer that referenced this pull request Mar 13, 2023
This commit adds a new file with the SHA1 of commits to ignore when
running git blame. This is important because of the recent adoption of
black and clang-format as our code formatting tool in Qiskit#1630 we caused a
large amount of code churn to change the code formatting. However, using
the ignore file is a local opt-in feature for git and not something we
can enable globally by default. To facilitate this a section is added to
the bottom of the contributing guide to document how this file can be
used.
@hhorii
Copy link
Collaborator Author

hhorii commented Mar 13, 2023

@mtreinish can we set this PR stable-backport-potential?

mergify bot pushed a commit that referenced this pull request Mar 14, 2023
This commit adds a new file with the SHA1 of commits to ignore when
running git blame. This is important because of the recent adoption of
black and clang-format as our code formatting tool in #1630 we caused a
large amount of code churn to change the code formatting. However, using
the ignore file is a local opt-in feature for git and not something we
can enable globally by default. To facilitate this a section is added to
the bottom of the contributing guide to document how this file can be
used.
hhorii added a commit to hhorii/qiskit-aer that referenced this pull request Apr 7, 2023
…++ (Qiskit#1630)

* add black and clang-format

* apply black and clang-format

* manually format codes that were not covered in black and clang-format

* add targets of clang-format and clean up tox.ini

* apply black with new configuration

* Update CONTRIBUTING.md

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Update .github/workflows/tests.yml

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* reformat qiskit_aer, test, tools, and setup.py with length 100

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
hhorii pushed a commit to hhorii/qiskit-aer that referenced this pull request Apr 7, 2023
This commit adds a new file with the SHA1 of commits to ignore when
running git blame. This is important because of the recent adoption of
black and clang-format as our code formatting tool in Qiskit#1630 we caused a
large amount of code churn to change the code formatting. However, using
the ignore file is a local opt-in feature for git and not something we
can enable globally by default. To facilitate this a section is added to
the bottom of the contributing guide to document how this file can be
used.
hhorii added a commit to hhorii/qiskit-aer that referenced this pull request Apr 7, 2023
…++ (Qiskit#1630)

* add black and clang-format

* apply black and clang-format

* manually format codes that were not covered in black and clang-format

* add targets of clang-format and clean up tox.ini

* apply black with new configuration

* Update CONTRIBUTING.md

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Update .github/workflows/tests.yml

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* reformat qiskit_aer, test, tools, and setup.py with length 100

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
hhorii pushed a commit to hhorii/qiskit-aer that referenced this pull request Apr 7, 2023
This commit adds a new file with the SHA1 of commits to ignore when
running git blame. This is important because of the recent adoption of
black and clang-format as our code formatting tool in Qiskit#1630 we caused a
large amount of code churn to change the code formatting. However, using
the ignore file is a local opt-in feature for git and not something we
can enable globally by default. To facilitate this a section is added to
the bottom of the contributing guide to document how this file can be
used.
hhorii added a commit that referenced this pull request Jun 13, 2023
* Bump version strings to prepare for release

* Remove redundant wheel dep from pyproject.toml (#1741)

Remove the redundant `wheel` dependency, as it is added by the backend automatically.

Listing it explicitly in the documentation was a historical mistake and has been fixed since,
see: [pypa/setuptools@f7d30a9](pypa/setuptools@f7d30a9).

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* add code-formatting with black for python and with clang-format for c++ (#1630)

* add black and clang-format

* apply black and clang-format

* manually format codes that were not covered in black and clang-format

* add targets of clang-format and clean up tox.ini

* apply black with new configuration

* Update CONTRIBUTING.md

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Update .github/workflows/tests.yml

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* reformat qiskit_aer, test, tools, and setup.py with length 100

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Add git blame ignore file (#1745)

This commit adds a new file with the SHA1 of commits to ignore when
running git blame. This is important because of the recent adoption of
black and clang-format as our code formatting tool in #1630 we caused a
large amount of code churn to change the code formatting. However, using
the ignore file is a local opt-in feature for git and not something we
can enable globally by default. To facilitate this a section is added to
the bottom of the contributing guide to document how this file can be
used.

* deploy documentation in qiskit.org/ecosystem (#1748)

* deploy documentation in qiskit.org/ecosystem

* comment

* Remove a release note in an invalid location (#1750)

Essentially equivalent content is already incorporated in #1739.

* Fix how to reference to config.cuStateVec_enable (#1749)

* Fix how to reference to config.cuStateVec_enable

* Add a release note

* Update fix-cuStateVec_enable-0936f2269466e3be.yaml

---------

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* use `omp_set_max_active_levels` instead of `omp_set_nested` (#1752)

omp_set_nested was deprecated since OpenMP 3.0 and this commit
replaces it with omp_set_max_active_levels.

* use omp_set_max_active_levels instead of omp_set_nested
* use omp_set_nested in WIN
* Update releasenotes/notes/use_omp_set_max_active_levels-7e6c1d301c4434a6.yaml
* use _OPENMP to select omp_set_max_active_levels or omp_set_nested
---------

Co-authored-by: Jake Lishman <jake@binhbar.com>

* Ensure QuantumCircuit.metadata is always a dict (#1761)

* Ensure QuantumCircuit.metadata is always a dict

This is for compatibility with Qiskit/qiskit#9849

* fix order

* fix typo :(

* revert object->object() change

* make backportable

* Add Tutorials to Documentation Page (#1768)

* Add tutorials

* Add pandoc installation

---------

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>

* Defer gathering backends until they are needed (#1760)

* Defer gathering backends until they are needed

* Disable the not-an-iterable warning

Pylint infers _get_backends to always return None, even if we add type
annotations. Suppress the warning.

* Add @staticmethod to AerProvider._get_backends

---------

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>

* Fix wrong set of parameters in circuits with barriers (#1775)

* Fix wrong set of parameters in circuits with barriers

`AerCircuit` is created from a circuit by iterating its instrucitons
while skpping barrier. This leads inconsistency with positions of
parameter bindings. This commit adds barrier instruction to the class
and keeps positions of parameter bindings.

* fix lint error in test

* remove unused variable in test

* Add API docs for QuantumCircuit methods (#1753)

* Add new doc for circuit method apis

* Remove QuantumCircuit description

---------

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>

* Resolve regression from introduction of AER::Config (#1747)

This commit reduces redundant copy in `AER::Transpile::CircuitOptimization`.
Since 0.12.0, configuration is `AER::Config` instead of `json_t`. This new class
has many fields and then its copy overheads become high. Reduction of
redundant copy improves performance especially for low-qubit simulation.

* Remove REQUIRED to find PythonLibs in FindPybind11 (#1786)

* pin the version of scikit-build to 0.17.2

* Pin scikit-build version by 0.17.2

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* remove REQUIRED to find PythonLibs in FindPybind11

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Remove Sampler.close, assert DeprecationWarning of opflow, and update dependency (#1804)

Since qiskit-terra 0.24, `Sampler` does not have `close()` but Aer's `Sampler` still have the method. This commit takes the method as well as upgrade of Python versions in tutorial tests from 3.7 to 3.8.

* rm close
* assert DeprecationWarning
* update dependency

* Fix setting MPI processes and ranks (#1808)

0.12.0 accidentally removed MPI support if application does not use qobj. This commit reverts the change.

* Fix library path to cuQuantum and cuTENSOR (#1801)

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>

* Activate jQuery in docs (#1802)

Sphinx 6 no longer activates jQuery by default but `qiskit_sphinx_theme` still uses it.
This commits enables jQuery by adding the theme to extensions in conf.py.

---------

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>

* fix cuQuantum static linking (#1812)

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>

* Update standard_errors.py (#1799)

There was a typo in the rendering of the mathematical text on Depolarizing Error Page (https://qiskit.org/ecosystem/aer/stubs/qiskit_aer.noise.depolarizing_error.html)

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* Add implicit cast of argument types (#1770)

* Add implicit cast of argument types

Since 0.12.0, AerConfig is used to configure simulation, which
is directly bound to a AER::Config object through pybind.
This change requires application to specify strictly correct types
of configuration options. This commit allows implicit casting
to arguments if application specifies them with wrong types.
This commit resolves #1754.

* does not warning in cast of numpy classes if they are compatible with expected type
* fix lint issue
* simplify class checking

* Fix handling of None in noise model construction from BackendV2 (#1818)

Fixes `NoiseModel.from_backend` not to fail when calling it with a V2 backend having
faulty qubits such that T1 and T2 are `None` in the `target.qubit_properties`. 

* fix handling of None in noise model construction from BackendV2
* add reno
* simplify a bit
* update docs

* Add Getting Started page (#1780)

This commit adds documentation "Getting Started" page.

* Add Get Started page
* Remove unused class

---------
Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>

* avoid kernel crash from blas call errors (#1791)

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* fix fail of qobj run without run_options (#1792)

* fix fail of qobj run without run_options

* fix lint error

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* Support parameterized global phase (#1814)

* Support parameterized global phase

Though `ParameterExpression` can be set to `QuantumCircuit.global_phase`,
Aer does not bind parameter values to it in simulation phase. This commit fixes
this problem by resolving values of `global_phase` with specified `parameter_binds`
in `AerSimulator.run`.

* define AER::CONFIG::GLOBAL_PHASE_POS and its pybind
* fix lint issues
---------
Co-authored-by: Jun Doi <doichan@jp.ibm.com>

* check parameter_binds arguments for parameterized circuits (#1817)

Aer raises an error if `parameter_binds` is not specified though circuits have parameters.
This resolves the following issue:
#1773

* Set the number of qubits with the coupling map (#1825)

* Set n_qubits from coupling map

* Add a test and release notes

* Fix lint

---------

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>

* Fixing some typos (#1827)

* Update aer_densitymatrix.py

* Update aer_statevector.py

* Update README.md

* Update CONTRIBUTING.md

---------

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>

* Use transpile and run instead of execute in docstring (#1830)

* use transpile and run instead of execute in docstring

* use backend.run instead of execute in README

* use fake_provider for noise_model example

* Revert edits on README.md

* Update examples in docstring for qiskit_aer.noise

* Fix depolarizing noise model example

* Remove incorrect markup in noise/__init__.py

---------

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>

* Fix typos in aer_simulator, qasm_simulator docs (#1833)

* Fix typos.

* Add missing aer_simulator options.

* Update reference to SaveProbabilities according comment to PR.

* Upgrade to qiskit_sphinx_theme 1.12 (#1822)

* Warn if approximation=False and shots=None in Estimator (#1823)

* fix 1820

* rm unnecessary comment

* update docs

---------

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>

* Fix the grouping index bug in Estimator (#1839)

* Fix the bug

* lint

* Validate parameters of each gate in a circuit (#1834)

This commit adds checks of arguments (A number of qubits, clbits,
and parameters) for each gate to prevent from unexpected memory access
when a user defines wrong custom gate with a number of a standard gate.

* validate parameters of each gate in a circuit
* fix lint error
* fix lint error

* Correct a type of variance from complex to real (#1767)

Previously Aer Estimator wrongly returned a complex value as variance of estimation values.
This commit fixes this behavior to return a real value.

* update version in docs/conf.py

* Do not use circuit metadata to internally manage simulation results (#1772)

* Stop using circuit metadata to internaly manage simulation results

This fixes `AerSimulator` to use circuit metadata to maintain mapping
from input and output of an executor call. This fixes an issue
#1723.

* add index of AER::Circuit and ExperimentResult

* add a link to an input circuit in each experiment result

* fix MPI build break (#1842)

* Batch QuantumCircuit with multiple parameter_binds (#1840)

`Estimator` in Aer did not use parameter bindings appropriately. Backend is called multiple times 
to simulate a circuit with multiple parameter-sets. This commit is to call backend once for multiple
parameter-sets for a circuit.

* WIP
* refactor
* lint
* add reno

* add release note with prelude

* *Support initialize with int (#1841)

Previously `QuantumCircuit.initialize` was not correctly preocessed
if initial state is specified with single `int` value. This commit
fixes this issue by decomposing initialize instructions.

---------

Co-authored-by: Sam James <sam@cmpct.info>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
Co-authored-by: derwind <uncertainty_principle_ss@yahoo.co.jp>
Co-authored-by: Jake Lishman <jake@binhbar.com>
Co-authored-by: Ian Hincks <ian.hincks@gmail.com>
Co-authored-by: Hitomi Takahashi <hitomi@jp.ibm.com>
Co-authored-by: Jonathan Coates <git@squiddev.cc>
Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
Co-authored-by: Jun Doi <doichan@jp.ibm.com>
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Co-authored-by: Deji Oyerinde <deji725@gmail.com>
Co-authored-by: Toshinari Itoko <15028342+itoko@users.noreply.github.com>
Co-authored-by: INNAN Nouhaila <64653897+Innanov@users.noreply.github.com>
Co-authored-by: Davide Gessa <gessadavide@gmail.com>
Co-authored-by: Oleksii Borodenko <kosunix@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge This PR will automatically merge once its CI has passed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants