some questions (dev branch) #55
Replies: 1 comment
-
first of all, thanks for the feedback on the update! The differences in the files
Yes I think throwing an error here would be a cleaner solution.
Yes specifying the type of the items expected would be nicer, but I am not sure whether we can do this for all occurrences.
No.
Yeah we can remove this import. Regarding # pylint: disable=R1728: Yes it would be nicer to move these lines to generators.
Yes we can remove that R0913. Overall we should take a look at the current disabled Pylint suggestions in
Yes seems like there is a bug. According to https://numpy.org/devdocs/reference/arrays.nditer.html#modifying-array-values we just need to add
Sorry, this is probably my mistake when integrating these changes in our internal QUARK 2.0 branch, which we developed in parallel and made rebasing quite tricky sometimes. Let us discuss the details in the next open call tomorrow. If you already have a commit ready with some of the fixes, it would be easiest if you open a PR, and we take care of the rest of the fixes. |
Beta Was this translation helpful? Give feedback.
-
I noticed that the recent changes and additions of QUARK 2.0 introduced some typos and such. I have a typo correcting commit almost ready. However, there are some things where I'm unsure how to handle them and would like to hear your opinion on.
The files
README.md
anddocs/tutorial.rst
have mostly the same content, but not completely. The differences I found are:README.md
is missing intutorial.rst
(this seems potentially intentional, since most of the missing part is indocs/index.rst
)--config test_config.yml
(README.md
) vs--config config.yml
(tutorial.rst
)tutorial.rst
tutorial.rst
README.md
in commit e02e3d8 ("Introduction of the Installer class"), but not intutorial.rst
README.md
Are (some of) these differences intentional or would it be better to equalize the two files in terms of content?
Further things that I have found:
utils.py
, method_get_instance_with_sub_options(...)
:If no matching class is found, it just ends (no return or anything). Shouldn't it rather throw an error?
According to PEP 484, you should specify the type of the items expected to be given to
**kwargs
, not the type of**kwargs
(dict
) itself, i.e.::type kwargs: dict
->:type kwargs: str
if strings are expected.
This may seem overly specific/pedantic, but PyCharm gave me "wrong type" warnings.
It's nothing major, though. If you think it's better to leave dict as the type hint, I would accept that.
Also see: https://youtrack.jetbrains.com/issue/PY-32949/args-and-kwargs-Wrong-hint-Expected-type-dict-got-str-instead
I think
Core.get_default_submodule(...)
should be made an abstract method and/or return aNotImplementedError
in the non-overridden form. Any objections?src/modules/applications/optimization/PVC/PVC.py
:There is an "unused"
import os
, becauseos
is already imported throughfrom modules.applications.Application import *
, which in turn imports it fromCore
in the same manner. => Do you mind if I deleteimport os
inPVC.py
? (The same issue occurs for other classes as well, which raises the same question for them.)src/modules/applications/optimization/PVC/PVC.py
, methodvalidate(...)
:Is there a reason why you do
# pylint: disable=R1728
(consider-using-generator), instead of using the generator as apparently suggested by pylint?And what is the reason to use
list(set(list([...])))
in the first place?# pylint: disable=R1728
is used again insrc/modules/solvers/GreedyClassicalPVC.py
and insrc/modules/solvers/ReverseGreedyClassicalPVC.py
(both times in the methodrun(...)
). The same question applies here.src/modules/solvers/PennylaneQAOA.py
, method_pseudo_decor(...)
->ret_fun(...)
->from time import time
:What is the reason for
# pylint: disable=R0913
(too many arguments) here?src/modules/applications/optimization/TSP/mappings/ISING.py
, method_convert_ising_to_qubo(...)
:I think there might be a bug here. The method iterates over
it
, assigning the next value tox
in each iteration.As far as I understand this,
x
is a simple number here (not any kind of reference). As a result, writing tox
doesn't write back intosolution
. Which means thatx
immediately gets overwritten again by the value of the next iteration and the loop does absolutely nothing. Or is there some Python magic at work that I don't know of?For some reason, some things I changed with my very first typo commit have found their way back into the code. I don't know how this happened, but I'll fix them again, I guess. For example:
BenchmarkManager.vizualize_results
->BenchmarkManager.visualize_results
Please tell me what you think is best regarding the points I mentioned and I'll incorporate them into my (coming) PR.
Beta Was this translation helpful? Give feedback.
All reactions