CodeQL fixes and minor refactoring #126

dylanbmorgan · 2025-12-04T11:19:14Z

No description provided.

coveralls · 2025-12-04T11:20:59Z

Pull Request Test Coverage Report for Build 19927157305

Details

0 of 0 changed or added relevant lines in 0 files are covered.
988 unchanged lines in 5 files lost coverage.
Overall coverage increased (+0.1%) to 32.565%

Files with Coverage Reduction	New Missed Lines	%
parameters.py	1	64.0%
cube.py	43	0.0%
output.py	106	73.21%
vibrations.py	124	0.0%
geometry.py	714	29.23%

Totals
Change from base Build 19838632644:	0.1%
Covered Lines:	1371
Relevant Lines:	4210

💛 - Coveralls

dfttoolkit/vibrations.py



-class AimsVibrations(Vibrations, AimsGeometry):  # pyright: ignore
+class AimsVibrations(Vibrations, AimsGeometry):


dfttoolkit/vibrations.py



-class VaspVibrations(Vibrations, VaspGeometry):  # pyright: ignore
+class VaspVibrations(Vibrations, VaspGeometry):


dfttoolkit/parameters.py

+        # for i, d in enumerate(edges[0]):
+        #     self.settings["edge"].append([d, *list(edges[1][i, :])])


dfttoolkit/parameters.py

+        # if len(self.type) > 0:
+        #     text += "output cube " + self.type + "\n"
+        # else:
+        #     warn("No cube type specified", stacklevel=2)
+        #     text += "output cube" + "CUBETYPE" + "\n"
+
+        # for key, values in self.settings.items():
+        #     for v in values:
+        #         text += "cube " + key + " "
+        #         if key in self.parsing_functions:
+        #             text += self.parsing_functions[key][1](v) + "\n"
+        #         else:
+        #             print(v)
+        #             text += v + "\n"


tests/test_base.py

                def _supported_files(self) -> dict:
                    return {"arbitrary_format": ".arb_format"}

+                def dummy_method() -> None:


tests/conftest.py


 @pytest.fixture(scope="session")
-def aims_calc_dir(run_aims, cwd) -> Generator[str, None, None] | str:  # noqa: ANN001
+def aims_calc_dir(run_aims: bool, cwd: Path) -> Generator[str, None, None] | str:


dfttoolkit/utils/run_utils.py

+
+    __name__: str
+
+    def __call__(self, *args: Any, **kwargs: Any) -> Any: ...


dfttoolkit/utils/run_utils.py

+    output_file: str = "aims.out",
+    calc_dir: str = "./",
+    force: bool = False,
+) -> Decorator: ...


dfttoolkit/utils/run_utils.py

+    output_file: str = "aims.out",
+    calc_dir: str = "./",
+    force: bool = False,
+) -> Wrapper: ...


Copilot

Pull request overview

This PR addresses CodeQL findings and performs minor refactoring across the codebase. The changes focus on improving type safety by removing pyright ignore comments, modernizing file handling with pathlib, and cleaning up unused code.

Key changes include:

Improved type annotations with Protocol and @overload patterns for better decorator typing
Removed pyright ignore comments by fixing underlying type issues
Modernized file path handling using pathlib.Path instead of os.path
Updated dependencies (ipython 9.7.0→9.8.0, loro 1.8.2→1.10.0, marimo 0.18.1→0.18.2, narwhals 2.12.0→2.13.0)
Cleaned up unused/unreachable code and dead logic branches
Fixed several spelling errors in comments and docstrings

Reviewed changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
uv.lock	Dependency version updates for ipython, loro, marimo, and narwhals packages
tests/test_utils.py	Refactored to use pathlib.Path and updated docstrings
tests/test_benchmarking.py	Added pytest.mark.skip decorator and TODO placeholder
tests/test_base.py	Added noqa comments and dummy methods to satisfy CodeQL checks
tests/conftest.py	Added type hints and corrected spelling in comments
dfttoolkit/vibrations.py	Removed pyright ignore comments throughout, but introduced incorrect type changes for time_step parameters
dfttoolkit/utils/run_utils.py	Added Protocol and overload for better decorator type safety
dfttoolkit/utils/periodic_table.py	Changed classproperty parameter names from cls to self (incorrect)
dfttoolkit/parameters.py	Simplified lambda expressions and commented out NotImplementedError code blocks
dfttoolkit/output.py	Refactored control flow and updated return type annotations
dfttoolkit/geometry.py	Removed unused variable assignments and function parameters
dfttoolkit/cube.py	Removed unused method with NotImplementedError
dfttoolkit/benchmarking.py	Changed from inheritance to composition pattern for AimsOutput
dfttoolkit/base.py	Fixed spelling in comment (file type → filetype)

Comments suppressed due to low confidence (1)

tests/test_base.py:168

Normal methods should have at least one parameter (the first of which should be 'self').

                def dummy_method() -> None:

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-04T11:24:41Z

dfttoolkit/vibrations.py

        velocities: npt.NDArray[np.float64],
        index_pair: tuple,
-        time_step: float,
+        time_step: int,


Changing time_step from float to int is likely incorrect. Time steps in physics simulations are typically floating-point values (e.g., 0.5 fs, 1.0 fs). The docstring on line 578 even says "time_step : float". This type change appears to be a mistake and could cause type errors when passing actual time step values.

Suggested change

time_step: int,

time_step: float,

Copilot · 2025-12-04T11:24:42Z

dfttoolkit/vibrations.py

    index: npt.NDArray,
    velocities_proj: npt.NDArray,
-    time_step: float,
+    time_step: int,


Changing time_step from float to int is likely incorrect. Time steps in physics simulations are typically floating-point values (e.g., 0.5 fs, 1.0 fs). This type change appears to be a mistake and could cause type errors when passing actual time step values.

Suggested change

time_step: int,

time_step: float,

Copilot · 2025-12-04T11:24:42Z

dfttoolkit/output.py

            dict[str, npt.NDArray[np.int64 | np.float64]],
            dict[str, npt.NDArray[np.int64 | np.float64]],
        ]
+        | None


Adding | None to the return type annotation is incorrect. The function always returns a value (either a dict or a tuple of dicts) after the refactoring. The else branch on lines 1359-1373 ensures all code paths return a value, so None should not be in the return type.

Suggested change

| None

Copilot · 2025-12-04T11:24:42Z

dfttoolkit/utils/periodic_table.py


    @classproperty
-    def elements(cls) -> dict[str, Element]:  # noqa: N805
+    def elements(self) -> dict[str, Element]:


The parameter name should be cls not self for a classproperty. While classproperty might handle this internally, using self for a class-level property is misleading and violates Python conventions. The original cls parameter name on lines 87 and 99 in the same file shows the correct pattern.

Copilot · 2025-12-04T11:24:43Z

dfttoolkit/utils/periodic_table.py


    @classproperty
-    def element_symbols(cls) -> list[str]:  # noqa: N805
+    def element_symbols(self) -> list[str]:


The parameter name should be cls not self for a classproperty. While classproperty might handle this internally, using self for a class-level property is misleading and violates Python conventions.

Copilot · 2025-12-04T11:24:44Z

dfttoolkit/utils/periodic_table.py


    @classproperty
-    def element_names(cls) -> list[str]:  # noqa: N805
+    def element_names(self) -> list[str]:


The parameter name should be cls not self for a classproperty. While classproperty might handle this internally, using self for a class-level property is misleading and violates Python conventions.

Copilot · 2025-12-04T11:24:44Z

dfttoolkit/parameters.py

+        # self.settings["edge"] = []
+        # for i, d in enumerate(edges[0]):
+        #     self.settings["edge"].append([d, *list(edges[1][i, :])])


This comment appears to contain commented-out code.

Suggested change

# self.settings["edge"] = []

# for i, d in enumerate(edges[0]):

# self.settings["edge"].append([d, *list(edges[1][i, :])])

Copilot · 2025-12-04T11:24:44Z

dfttoolkit/parameters.py

+        # text = ""
+        # if len(self.type) > 0:
+        #     text += "output cube " + self.type + "\n"
+        # else:
+        #     warn("No cube type specified", stacklevel=2)
+        #     text += "output cube" + "CUBETYPE" + "\n"
+
+        # for key, values in self.settings.items():
+        #     for v in values:
+        #         text += "cube " + key + " "
+        #         if key in self.parsing_functions:
+        #             text += self.parsing_functions[key][1](v) + "\n"
+        #         else:
+        #             print(v)
+        #             text += v + "\n"
+
+        # return text


This comment appears to contain commented-out code.

Suggested change

# text = ""

# if len(self.type) > 0:

# text += "output cube " + self.type + "\n"

# else:

# warn("No cube type specified", stacklevel=2)

# text += "output cube" + "CUBETYPE" + "\n"

# for key, values in self.settings.items():

# for v in values:

# text += "cube " + key + " "

# if key in self.parsing_functions:

# text += self.parsing_functions[key][1](v) + "\n"

# else:

# print(v)

# text += v + "\n"

# return text

Copilot · 2025-12-04T11:24:44Z

dfttoolkit/vibrations.py



-class AimsVibrations(Vibrations, AimsGeometry):  # pyright: ignore
+class AimsVibrations(Vibrations, AimsGeometry):


Base classes have conflicting values for attribute 'get_instance_of_other_type': Function get_instance_of_other_type and Function get_instance_of_other_type.

Copilot · 2025-12-04T11:24:45Z

dfttoolkit/vibrations.py



-class VaspVibrations(Vibrations, VaspGeometry):  # pyright: ignore
+class VaspVibrations(Vibrations, VaspGeometry):


Base classes have conflicting values for attribute 'get_instance_of_other_type': Function get_instance_of_other_type and Function get_instance_of_other_type.

dylanbmorgan added 4 commits December 3, 2025 14:45

Added fixes for most CodeQL findings

8011081

Commented unfinished tests

79827f5

Further corrections

483cb71

Further logic, style, and quality fixes/improvements

f3dc6c3

dylanbmorgan requested review from Copilot and lukas-hoermann December 4, 2025 11:19

Copilot started reviewing on behalf of dylanbmorgan December 4, 2025 11:19 View session

dylanbmorgan linked an issue Dec 4, 2025 that may be closed by this pull request

Fix security standard findings #124

Open

github-code-quality bot found potential problems Dec 4, 2025

View reviewed changes

Copilot finished reviewing on behalf of dylanbmorgan December 4, 2025 11:22

dylanbmorgan marked this pull request as draft December 4, 2025 11:23

Copilot AI reviewed Dec 4, 2025

View reviewed changes

dylanbmorgan self-assigned this Dec 9, 2025

dylanbmorgan closed this Dec 22, 2025



		class AimsVibrations(Vibrations, AimsGeometry): # pyright: ignore
		class AimsVibrations(Vibrations, AimsGeometry):



		class VaspVibrations(Vibrations, VaspGeometry): # pyright: ignore
		class VaspVibrations(Vibrations, VaspGeometry):

		# for i, d in enumerate(edges[0]):
		# self.settings["edge"].append([d, *list(edges[1][i, :])])


		__name__: str

		def __call__(self, args: Any, *kwargs: Any) -> Any: ...

	# self.settings["edge"] = []
	# for i, d in enumerate(edges[0]):
	# self.settings["edge"].append([d, *list(edges[1][i, :])])

CodeQL fixes and minor refactoring #126

CodeQL fixes and minor refactoring #126

Uh oh!

Conversation

dylanbmorgan commented Dec 4, 2025

Uh oh!

coveralls commented Dec 4, 2025

Pull Request Test Coverage Report for Build 19927157305

Details

💛 - Coveralls

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants