Cantera / enhancements

Repository for proposed and ongoing enhancements to Cantera
11 stars 5 forks source link

Increase the usage of augmented assignment statements #121

Closed elfring closed 2 years ago

elfring commented 2 years ago

Abstract :eyes: Some source code analysis tools can help to find opportunities for improving software components. :thought_balloon: I propose to increase the usage of augmented assignment statements accordingly.

Motivation Rationale of the Python enhancement proposal 203 (from 2000-07-13)

Possible Solutions Would you like to integrate anything from a transformation result which can be generated by a command like the following? (:point_right: Please check also for questionable change suggestions because of an evolving search pattern.)

lokal$ perl -p -i.orig -0777 -e 's/^(?<indentation>\s+)(?<target>\S+)\s*=\s*\k<target>[ \t]*(?<operator>[+\-%&|^@]|\*\*?|\/\/?|<<|>>)/$+{indentation}$+{target} $+{operator}=/gm' $(find ~/Projekte/Cantera/lokal -name '*.py')
elfring commented 2 years ago

:thought_balloon: Can change suggestions like the following trigger more constructive software development considerations?

diff --git a/interfaces/cython/cantera/examples/onedim/diffusion_flame_extinction.py b/interfaces/cython/cantera/examples/onedim/diffusion_flame_extinction.py
index 8ef18c8d6..f3f201318 100644
--- a/interfaces/cython/cantera/examples/onedim/diffusion_flame_extinction.py
+++ b/interfaces/cython/cantera/examples/onedim/diffusion_flame_extinction.py
@@ -163,7 +163,7 @@ while True:
     else:
         # Procedure if flame extinguished but abortion criterion is not satisfied
         # Reduce relative strain rate increase
-        delta_alpha = delta_alpha / delta_alpha_factor
+        delta_alpha /= delta_alpha_factor

         print('Flame extinguished at alpha = {0:8.4F}. Restoring alpha = {1:8.4F} and '
               'trying delta_alpha = {2}'.format(
diff --git a/interfaces/cython/cantera/onedim.py b/interfaces/cython/cantera/onedim.py
index 04ed26ce1..c7fd8c2cd 100644
--- a/interfaces/cython/cantera/onedim.py
+++ b/interfaces/cython/cantera/onedim.py
@@ -643,7 +643,7 @@ class FlameBase(Sim1D):
                 'energy_enabled', 'soret_enabled', 'radiation_enabled',
                 'fixed_temperature',
                 'max_time_step_count', 'max_grid_points'}
-        attr = attr & set(s.keys())
+        attr &= set(s.keys())
         for key in attr:
             setattr(self, key, s[key])

diff --git a/interfaces/cython/cantera/test/test_onedim.py b/interfaces/cython/cantera/test/test_onedim.py
index 7a0f2d37b..4fa225732 100644
--- a/interfaces/cython/cantera/test/test_onedim.py
+++ b/interfaces/cython/cantera/test/test_onedim.py
@@ -1409,7 +1409,7 @@ class TestTwinFlame(utilities.CanteraTest):
         arr = sim.to_solution_array()
         axial_velocity = 2.2
         sim.reactants.mdot *= 1.1
-        sim.reactants.T = sim.reactants.T + 100
+        sim.reactants.T += 100
         sim.set_initial_guess(data=arr)
         sim.solve(loglevel=0, auto=True)

diff --git a/site_scons/buildutils.py b/site_scons/buildutils.py
index 6fa04854e..9c646f926 100644
--- a/site_scons/buildutils.py
+++ b/site_scons/buildutils.py
@@ -253,7 +253,7 @@ class TestResults:
             message = (message + "Failed tests:" +
                        "".join("\n    - " + n for n in self.failed) +
                        "\n")
-        message = message + "*****************************"
+        message += "*****************************"
         if self.failed:
             logger.error("One or more tests failed.\n" + message, print_level=False)
             sys.exit(1)
@@ -415,7 +415,7 @@ def compare_text_files(env: "SCEnvironment", file1: Path, file2: Path) -> TestRe
                 pass
             else:
                 precision = max(get_precision(float_1[1]), get_precision(float_2[1]))
-                atol = atol + pow(10, precision) * 1.1
+                atol += pow(10, precision) * 1.1
                 abserr = abs(num1 - num2)
                 relerr = abserr / (0.5 * abs(num1 + num2) + atol)
                 if abserr > atol and relerr > rtol:
bryanwweber commented 2 years ago

Hi @elfring! Thanks for making this suggestion. What advantages does this change have? Is the code faster? I don't find it necessarily easier to read, and this would cause a lot of churn in our code...

elfring commented 2 years ago

I find that the rationale of the Python enhancement proposal 203 (from 2000-07-13) can indicate also motivation for another bit of collateral evolution. :thinking:

bryanwweber commented 2 years ago

@elfring thanks for the link! What about the rationale applies here? Are there performance implications that can be demonstrated to justify the code churn?

ischoegl commented 2 years ago

I think that +=, etc. completes manipulations in-place and avoids memory assignment/copying operations, and I have seen big differences for large arrays of data. On scalars, I am unsure whether it makes a big difference. I personally find code easy to read, but I am not sure that it is worth the churn.

PS: Code churn is moderate, running the perl script yields 6 replacements on current main ... :smile:

PPS: I'd also suggest not running that script as it created all kinds of garbage that I now get to clean out. A simple git stash won't do ... :angry: ... Edit: not too bad after all with `$ find . -name '.orig' | xargs rm`*

ischoegl commented 2 years ago

Closing, as changes appear to be trivial.

elfring commented 2 years ago

:crystal_ball: Will application interests ever grow for functionality which became available with Python 2.0 (on 2000-10-16)?

bryanwweber commented 2 years ago

@elfring Based on what @ischoegl said, the changes appear to be fairly minimal, please feel free to submit a pull request.

ischoegl commented 2 years ago

@bryanwweber ... this account is likely a bot, as reponses are gibberish.