Skip to content

Comments

pi-thon 3.14 fixes#75

Open
mimakaev wants to merge 14 commits intomasterfrom
mi/3.14_fixes
Open

pi-thon 3.14 fixes#75
mimakaev wants to merge 14 commits intomasterfrom
mi/3.14_fixes

Conversation

@mimakaev
Copy link
Collaborator

@mimakaev mimakaev commented Oct 26, 2025

Description

This PR contains fixes for the python 3.14 switching from fork to forkserver multiprocessing method, addresses some deprecation warnings, and reformats things with black.

  • Changed CI to include 3.14 and two latest versions of MacOS
  • black and isort on the codebase
  • Switched contactmaps to be compatible with spawn and forkserver multiprocessing (latest default in 3.14)
  • removed confusing test for an old starting conformation generator
  • Addressed all flake8 issues and all valid VSCode warnings such as possibly unbound variables
  • Deprecated some unnecessary functions, like streaming ndarray agg
  • redid imports from simtk.unit due to dynamic module nature (pylance errors)

PR Checklist

  • [] apply "black" to the whole codebase (black .)
  • [] apply isort to the codebase (isort .)
  • [] fun flake8 and try to resolve all the issues (work in progress!)

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the polychrom codebase for Python 3.14 compatibility, focusing on multiprocessing method changes (fork → forkserver), addressing deprecation warnings, and applying code formatting with black/isort.

Key Changes:

  • Updated CI/CD to test against Python 3.14 and recent macOS versions
  • Refactored contactmaps module for spawn/forkserver multiprocessing compatibility (lambda functions → named functions)
  • Fixed NumPy deprecation (np.in1dnp.isin) and improved type checking patterns
  • Applied consistent code formatting across the codebase

Reviewed Changes

Copilot reviewed 24 out of 25 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
.github/workflows/pythonapp.yml Added Python 3.14 and macOS testing matrix
polychrom/contactmaps.py Refactored for multiprocessing compatibility: moved lambdas to module-level functions, added default function parameters
tests/test_contactmaps.py Replaced lambda with named load_function for spawn/forkserver compatibility; added module-level test data
polychrom/forces.py Updated cylindrical_confinement logic and type hints; contains a bug with bottom=0 handling
polychrom/forcekits.py Added explicit error for unsupported nonbonded forces (potential breaking change)
tests/test_polymer_analyses.py Fixed NumPy deprecation: np.in1dnp.isin
tests/test_io.py Updated to use new load_URI API instead of deprecated fetch_block
polychrom/polymer_analyses.py Added TYPE_CHECKING pattern for better Cython import handling
polychrom/param_units.py Reformatted docstrings with black (r""" prefix)
polychrom/hdf5_format.py Changed type() to isinstance() for better practices
polychrom/legacy/forces.py Reordered imports and changed to isinstance()
Various files Import reordering (isort), spacing fixes, removed extra blank lines (black formatting)
tests/test_configurations.py Removed entire test file for old starting conformation generator
Examples Import reordering and formatting improvements

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

bottom = 0 if bottom is True else bottom

if bottom is not None:
if bottom:
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

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

The logic change here introduces a bug. When bottom=0 is passed, the condition if bottom: (line 734) will evaluate to False because 0 is falsy in Python, causing the code to use the wrong branch.

The original code correctly used if bottom is not None: which would handle bottom=0 properly. The fix should be:

bottom = 0 if bottom is True else bottom

if bottom is not None:
    # ... existing code for bottom confinement
Suggested change
if bottom:
if bottom is not None:

Copilot uses AI. Check for mistakes.
Comment on lines 130 to 131
else:
raise ValueError("Nonbonded force {} does not support exceptions or exclusions.".format(nb_force.name))
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

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

This change alters the behavior when except_bonds=True and the nonbonded force doesn't support exceptions or exclusions. Previously, the code would silently continue without adding exceptions. Now it raises a ValueError.

While this is likely an improvement (making the error explicit rather than silent), it could break existing code that relies on the previous behavior. Consider documenting this breaking change or verifying that all nonbonded forces in use support either exceptions or exclusions.

Copilot uses AI. Check for mistakes.
@@ -1 +1 @@
from polychrom.polymerutils import * # noqa: F403
from polychrom.polymerutils import * # noqa: F403
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

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

Import pollutes the enclosing namespace, as the imported module polychrom.polymerutils does not define 'all'.

Suggested change
from polychrom.polymerutils import * # noqa: F403
# TODO: Replace the following line with explicit imports of only the required names.
# For example: from polychrom.polymerutils import foo, bar
# from polychrom.polymerutils import * # noqa: F403

Copilot uses AI. Check for mistakes.
@mimakaev
Copy link
Collaborator Author

unused imports and other things will be addressed in a next PR

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 25 out of 26 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

polychrom/param_units.py:189

  • guess_bondWiggleDistance doesn’t use self but is now an instance method, which is a silent breaking change for any code calling it as a class/static utility (e.g. SimulationParams.guess_bondWiggleDistance(...)). Consider making it a @staticmethod again (or accepting both calling styles) to preserve the previous API.
    def guess_bondWiggleDistance(self, L0, b, mean_linker_length, a=None):
        """Return bond wiggle distance based on the amount of DNA per bead (L0), the
        Kuhn length (b) in basepairs, and the mean linker length in basepairs, and the
        expected radius of a monomer in nanometers (a)."""
        L0_nm = L0 / (1 + 146 / mean_linker_length) * 0.34
        b_nm = b / (1 + 146 / mean_linker_length) * 0.34
        if a is None:
            a = b_nm
        return np.sqrt(2 * L0_nm * b_nm / 3) / a

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

Comment on lines 256 to 259
for file in files:
try:
h5py.File(file, "r")
except Exception:
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

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

list_URIs opens each HDF5 file with h5py.File(file, "r") but never closes it. Use a context manager to ensure file handles are released (or explicitly close) to avoid leaking descriptors during large directory scans.

Copilot uses AI. Check for mistakes.
Comment on lines 97 to 98
import openmm # if this fails, update openmm

Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

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

simulation.py now hard-imports openmm with no fallback to simtk.openmm. Since OpenMM isn’t listed in requirements.txt and other modules still support the legacy import path, this can break environments that only have simtk.openmm installed. Consider restoring the previous try/except import fallback (or updating installation requirements consistently across the project).

Suggested change
import openmm # if this fails, update openmm
try:
import openmm # preferred OpenMM namespace
except ImportError: # fall back to legacy OpenMM package layout
from simtk import openmm # type: ignore[no-redef]

Copilot uses AI. Check for mistakes.
Comment on lines +616 to +620
except ImportError:
warnings.warn(
"C++ simplification module not available. " "Please compile the Cython extensions.", RuntimeWarning
)
return data
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

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

This only catches ImportError, but if _polymer_math failed to import at module import time (the top-level try/except does pass), calling _polymer_math.simplifyPolymer(...) will raise NameError, not ImportError, and won’t be handled. Consider setting _polymer_math = None when import fails and/or catching NameError here so the intended warning + fallback path works.

Copilot uses AI. Check for mistakes.
mimakaev and others added 4 commits February 18, 2026 12:08
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.

1 participant