Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix in-cell slots #126

Merged
merged 10 commits into from
Sep 23, 2023
+2,452 −2,104
Merged

Fix in-cell slots #126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2a77031
refactor slot methods
hanjinliu Sep 17, 2023
25d1493
fix empty check
hanjinliu Sep 18, 2023
e8ae914
fix typing, add more tests
hanjinliu Sep 18, 2023
8a38478
fix plot
hanjinliu Sep 18, 2023
1f2c044
improve type safety
hanjinliu Sep 19, 2023
a0dd108
add additional ranges for showin g slot ranges
hanjinliu Sep 21, 2023
68f90c6
improve plot UI
hanjinliu Sep 21, 2023
12e08e1
skip the line to avoid failing on ubuntu
hanjinliu Sep 22, 2023
d0156f3
minor update in startup
hanjinliu Sep 22, 2023
c3bec90
bug fix of using N
hanjinliu Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions tabulous/__main__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ def main():
TXT_PATH.write_text("")

from . import TableViewer
from ._async_importer import import_plt, import_scipy
from ._qt._console import import_qtconsole_threading

viewer = TableViewer()
Expand All @@ -91,9 +90,6 @@ def main():
viewer.open(args.open_file)

import_qtconsole_threading()
import_plt()

import_scipy()
viewer.show()
return

Expand Down
28 changes: 8 additions & 20 deletions tabulous/_async_importer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import threading
from types import ModuleType
from typing import Callable, Generic, TypeVar
import concurrent.futures

THREAD: "threading.Thread | None" = None

_T = TypeVar("_T")

Expand All @@ -22,26 +21,15 @@ class AsyncImporter(Generic[_T]):

def __init__(self, import_func: Callable[[], _T]) -> None:
self._target = import_func
self._thread: "threading.Thread | None" = None
self._future: "concurrent.futures.Future[_T] | None" = None

def run(self) -> None:
with threading.Lock():
if self._thread is None:
self._thread = threading.Thread(target=self._target, daemon=True)
self._thread.start()
else:
self._thread.join()

def get(self, ignore_error: bool = True) -> _T:
try:
self.run()
except Exception as e:
if ignore_error:
return None
else:
raise e
else:
return self._target()
if self._future is None or self._future.done():
self._future = concurrent.futures.ThreadPoolExecutor().submit(self._target)
Copy link

Choose a reason for hiding this comment

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

Creating a new ThreadPoolExecutor each time the run() method is called can be inefficient and lead to resource leaks if the executor is not properly shut down. It would be better to create a single executor instance and reuse it for all tasks. Here's how you might do this:

- self._future = concurrent.futures.ThreadPoolExecutor().submit(self._target)
+ if not hasattr(self, '_executor'):
+     self._executor = concurrent.futures.ThreadPoolExecutor()
+ self._future = self._executor.submit(self._target)


def get(self, timeout: float = None) -> _T:
self.run()
return self._future.result(timeout)

__call__ = get

Comment on lines 21 to 35
Copy link

Choose a reason for hiding this comment

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

The AsyncImporter class has been refactored to use a ThreadPoolExecutor from the concurrent.futures module instead of creating new threads with the threading module. The run() method now submits the import function to the executor, and the get() method retrieves the result of the future. This approach should improve performance by reusing threads in the pool and avoiding the overhead of creating new threads. However, it's important to ensure that the number of concurrent tasks does not exceed the number of available threads in the pool, as this could lead to contention and degrade performance.

Expand Down
2 changes: 1 addition & 1 deletion tabulous/_magicgui/_dialog.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ def _runner(parent=None, **param_options):
style = "dark_background"
bg = viewer._qwidget.backgroundColor().name()

plt = QtMplPlotCanvas(style=style)
plt = QtMplPlotCanvas(style=style, pickable=False)
if bg:
plt.set_background_color(bg)

Expand Down
10 changes: 0 additions & 10 deletions tabulous/_magicgui/_selection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,16 +77,6 @@ def value(self, val: str | SelectionOperator) -> None:
def format(self) -> str:
return self._format

def as_iloc(self) -> tuple[slice, slice]:
"""Return current value as a indexer for ``iloc`` method."""
df = self._find_table().data_shown
return self.value.as_iloc(df)

def as_iloc_slices(self) -> tuple[slice, slice]:
"""Return current value as slices for ``iloc`` method."""
df = self._find_table().data_shown
return self.value.as_iloc_slices(df)

def _find_table(self) -> TableBase:
table = find_current_table(self)
if table is None:
Comment on lines 81 to 82
Copy link

Choose a reason for hiding this comment

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

The _find_table method is missing error handling for when table is None. This could lead to a NoneType error if the method is called elsewhere and the return value is used without checking for None. Consider raising an exception or returning a default value when table is None.

     def _find_table(self) -> TableBase:
         table = find_current_table(self)
         if table is None:
+               raise ValueError("No current table found.")

Expand Down
Loading