Modularize Frontend

[pauladkisson]

Fixes #173

General goals include

• decoupling dedicated front-end to tkinter or Panel code from an orchestration layer that ties everything together

• Refactoring highly stateful blocks of code into classes

• Refactoring highly functional blocks of code into pure functions

• Reorganizing frontend blocks into dedicated modules

• Making functional relationships between coding sections as explicit as possible (ex. explicit parameters instead of implicit closures where possible)

• De-duplicate code where applicable

• SavingInputParameters.py --> home.py (home)

• SavingInputParameters.py --> save_parameters.py (step 1)

• saveStoresList.py --> storenames.py (step 2)

• readTevTsq.py --> read_raw_data.py (step 3)

• preprocess.py (step 4)

• computePsth.py --> psth.py (step 5)

• findTransientsFreqAndAmp.py --> transients.py (step 5)

• visualizePlot.py --> visualize.py (step 6)

[claude]

PR Review: Modularize Frontend

This is a substantial and well-motivated refactor. The separation of concerns across frontend/, orchestration/, visualization/, and utils/ is a clear improvement over the previous monolithic structure. Below are observations across several dimensions.

---

Architecture & Design (Positive)

• Clear layering: splitting Panel/tkinter UI into frontend/, business logic into orchestration/, and rendering into visualization/ is the right call and aligns with the stated goals.
• De-duplication: consolidating writeToFile, takeOnlyDirs, get_all_stores_for_combining_data, and read_Df into canonical locations removes real duplication that existed before.
• Typo fix: visuzlize_peaks → visualize_peaks is appreciated.
• __init__-based injection: ParameterForm.__init__ using keyword-only args (*) for template and folder_path is a good practice.

---

Issues & Concerns

1. scanPortsAndFind has an infinite loop risk (frontend/frontend_utils.py:8)

def scanPortsAndFind(start_port=5000, end_port=5200, host="127.0.0.1"):
    while True:
        port = randint(start_port, end_port)
        sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
        sock.settimeout(0.001)
        result = sock.connect_ex((host, port))
        if result == 0:
            continue
        else:
            break
    return port

Two problems:

• If all 200 ports in the range are occupied, this loops forever.
• The socket is never closed. Each iteration leaks a file descriptor.

Suggested fix:

def scanPortsAndFind(start_port=5000, end_port=5200, host="127.0.0.1"):
    ports = list(range(start_port, end_port + 1))
    random.shuffle(ports)
    for port in ports:
        with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock:
            sock.settimeout(0.001)
            if sock.connect_ex((host, port)) != 0:
                return port
    raise RuntimeError(f"No free port found in range {start_port}-{end_port}")

2. == True anti-pattern (frontend/artifact_removal.py:25)

if (y1 == 0).all() == True:

Should be:

if (y1 == 0).all():

The == True comparison is redundant and a common Python code smell.

3. _on_close double-checks self.coords redundantly (frontend/artifact_removal.py:61)

if self.coords and len(self.coords) > 0:

if self.coords: already checks for non-empty. The len(...) > 0 is redundant.

4. checkSameLocation has a commented-out line left in (frontend/input_parameters.py:12)

def checkSameLocation(arr, abspath):
    # abspath = []

The # abspath = [] comment appears to be stale debugging code. The function receives abspath as a mutable argument and appends to it — this is a non-obvious side-effect pattern. Consider either making it a local variable (and returning the result) or documenting the intent clearly. As written, callers must pass a list they've already created, which is implicit.

5. getAbsPath duplicates the "all folders at same location" error (frontend/input_parameters.py:19-38)

The identical error message and exception are raised in both checkSameLocation and getAbsPath after the np.unique call. The second check in getAbsPath is unreachable: if checkSameLocation raises for len > 1, control never returns to the duplicate check. If it is intended as an extra safety net, a comment explaining why would help.

6. pn.extension() called at module level in multiple files

storenames_config.py, storenames_instructions.py, and parameterized_plotter.py each call pn.extension() at import time. Panel recommends calling this once at application startup, not in every module. Multiple calls can cause issues (double-initialization of JS extensions, redundant notebook output). Centralise this call in home.py or the application entry point.

7. results_hm and results_psth are mutable class-level attributes (frontend/parameterized_plotter.py:66-67)

class ParameterizedPlotter(param.Parameterized):
    ...
    results_hm = dict()
    results_psth = dict()

These are class-level dicts, meaning all instances share the same dict. This is almost certainly unintentional. They should be initialised in __init__:

def __init__(self, **params):
    super().__init__(**params)
    self.results_hm = {}
    self.results_psth = {}
    ...

8. update_selector references arr[i] after the loop (frontend/parameterized_plotter.py)

for i in range(len(arr)):
    ...
if len(arr) > 0:
    if "bin" in arr[i]:   # <-- 'i' is the last loop index

This works coincidentally (Python keeps the loop variable after the loop), but it is fragile and hard to read. Using arr[-1] or the last element explicitly would be clearer.

9. save_opts variables are assigned but never used in several plot methods

In contPlot, plot_specific_trials, heatmap, and update_selector:

save_opts = self.save_options  # assigned but never passed anywhere

The actual save logic is commented out (# self.save_plots(...)). This creates dead variable assignments throughout. Either remove the assignment or restore the save calls.

10. Magic number 3 used for downsampling (frontend/parameterized_plotter.py)

index = np.arange(0, xpoints.shape[0], 3)

This 3 (take every 3rd sample for display) appears at least 5 times across contPlot, update_selector, and plot_specific_trials. Extract it as a named constant:

DISPLAY_DOWNSAMPLE_FACTOR = 3

11. Progress tracking via a home-directory file is fragile (frontend/progress.py:46-48)

def writeToFile(value: str):
    with open(os.path.join(os.path.expanduser("~"), "pbSteps.txt"), "a") as file:
        file.write(value)

Writing progress state to a hardcoded ~/pbSteps.txt file is problematic:

• Multiple simultaneous GuPPy sessions would collide.
• The file persists across sessions if a crash occurs.
• It uses filesystem polling with time.sleep(0.001) as an IPC mechanism.

This is pre-existing design debt rather than a regression introduced by this PR, but now that progress.py is its own module, this would be a good time to note it for future work (e.g., use a multiprocessing.Queue or Panel's built-in threading support).

12. npm_gui_prompts.py: tkinter messagebox called after window.destroy() (frontend/npm_gui_prompts.py)

In get_multi_event_responses:

window = tk.Tk()
response = messagebox.askyesno(...)
window.destroy()

messagebox.askyesno is called on the Tk root (window), then the root is destroyed. This is the correct order, but the pattern of creating a bare tk.Tk() root just to host a dialog is fragile on some platforms (the root window briefly appears). A more robust approach is window.withdraw() before showing the dialog.

13. Missing sock.close() / context manager in scanPortsAndFind (see #1 above — also affects the non-infinite-loop case)

Even in the happy path, the current code creates a socket and relies on the GC to close it. This should use with socket.socket(...) as sock:.

---

Minor Style Notes

• Function names mix camelCase (checkSameLocation, getAbsPath, scanPortsAndFind) with snake_case (get_folder_path, remove_cols, make_dir). PEP 8 prefers snake_case for functions. This is pre-existing in the codebase, but worth noting for new code added in this PR.
• frontend/input_parameters.py still uses for i in range(len(arr)): style loops in checkSameLocation — for item in arr: would be more Pythonic.

---

Test Coverage

The testing/api.py changes are import-path updates only. No new tests cover the new frontend/ or visualization/ modules. Given the complexity of ParameterizedPlotter and StorenamesConfig, at least smoke tests (can the classes be instantiated with mock Panel objects?) would add confidence. This is not a blocker for a refactor PR, but worth tracking.

---

Summary

Category	Finding	Severity
Bug	results_hm/results_psth shared class-level dicts	Medium
Bug	Socket leak + infinite loop in scanPortsAndFind	Medium
Bug	arr[i] post-loop reference in update_selector	Low
Code quality	pn.extension() called at module level multiple times	Medium
Code quality	== True comparison with numpy bool	Low
Code quality	Dead save_opts variable assignments	Low
Code quality	Magic 3 for downsampling repeated 5x	Low
Code quality	Commented-out # abspath = [] line	Low
Design	~/pbSteps.txt IPC pattern (pre-existing)	Low (note)

The refactor direction is sound. Addressing the shared mutable class attrs (#7) and the socket issues (#1, #13) before merge would be worthwhile.