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.

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

ncols Out of bounds error for GridSpec with fixed ncols when adding rows only. #1721

Closed
MarcSkovMadsen opened this issue Oct 30, 2020 · 3 comments · Fixed by #1733
Closed
Labels
type: bug Something isn't correct or isn't working
Milestone

Comments

@MarcSkovMadsen
Copy link
Collaborator

MarcSkovMadsen commented Oct 30, 2020

I'm on Panel 0.10.0

If I run pytest on the below

import panel as pn

def test_gridspec_ncols():
    grid = pn.layout.GridSpec(ncols=3)
    for index in range(0,5):
        grid[index,:]=pn.pane.Markdown("Hello World")
$ pytest 'scripts\test_gridspec.py'
Test session starts (platform: win32, Python 3.7.6, pytest 5.4.2, pytest-sugar 0.9.3)
rootdir: C:\repos\private\awesome-panel, inifile: pytest.ini
plugins: cov-2.8.1, mock-3.1.0, sugar-0.9.3
collecting ... 

――――――――――――――――――――――――――― test_gridspec_ncols ――――――――――――――――――――――――――――

    def test_gridspec_ncols():
        grid = pn.layout.GridSpec(ncols=3)
        for index in range(0,5):
>           grid[index,:]=pn.pane.Markdown("Hello World")

scripts\test_gridspec.py:6:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = GridSpec(ncols=3, nrows=4)
    [0] Markdown(str)
    [1] Markdown(str)
    [2] Markdown(str)
    [3] Markdown(str)
index = (4, slice(None, None, None)), obj = Markdown(str)

    def __setitem__(self, index, obj):
        from ..pane.base import panel
        if not isinstance(index, tuple):
            raise IndexError('Must supply a 2D index for GridSpec assignment.')

        yidx, xidx = index
        if isinstance(xidx, slice):
            x0, x1 = (xidx.start, xidx.stop)
        else:
            x0, x1 = (xidx, xidx+1)

        if isinstance(yidx, slice):
            y0, y1 = (yidx.start, yidx.stop)
        else:
            y0, y1 = (yidx, yidx+1)

        l = 0 if x0 is None else x0
        r = self.nrows if x1 is None else x1
        t = 0 if y0 is None else y0
        b = self.ncols if y1 is None else y1

        if self._cols_fixed and (l >= self.ncols or r > self.ncols):
>           raise IndexError('Assigned object to column(s) out-of-bounds '
                             'of the grid declared by `ncols`. which '
                             f'was set to {self.ncols}.')
E           IndexError: Assigned object to column(s) out-of-bounds of the grid declared by `ncols`. which was set to 3.

.venv\lib\site-packages\panel\layout\grid.py:416: IndexError

 scripts/test_gridspec.py ⨯                                   100% ██████████
========================= short test summary info ==========================
FAILED scripts/test_gridspec.py::test_gridspec_ncols - IndexError: Assigne...

Results (1.47s):
       1 failed
         - scripts/test_gridspec.py:3 test_gridspec_ncols

I would expect to be able to add any number of rows. I only set a limit on the number of columns.

Additional Context

I use a similar script on awesome-panel.org to convert from a ListLike specification of panels to a GridSpec such that I can use all templates together on awesome-panel.org.

@MarcSkovMadsen MarcSkovMadsen added the TRIAGE Default label for untriaged issues label Oct 30, 2020
@hoxbro
Copy link
Member

hoxbro commented Oct 31, 2020

I think the problem is related to r and b. If i change the lines above the IndexError to the following, I can run the code.

 l = 0 if x0 is None else x0
 r = self.ncols if x1 is None else x1
 t = 0 if y0 is None else y0
 b = self.nrows if y1 is None else y1

I have a hard time figure out how the naming works, it would be nice with a docstring telling what the names means.
But for me it make sense that r should be related to self.ncols when I look at the if-statement and the same for b and self.nrows.

If I have understood it correct, I can make a PR for it. :)

@philippjfr
Copy link
Member

Sorry I'm so used to lbrt being abbreviations for left, bottom, right, top of a bounding box that I didn't think about leaving docstrings.

@hoxbro
Copy link
Member

hoxbro commented Oct 31, 2020

That make sense. Properly just me who didn't knew that...

Should I make a PR for this?

@philippjfr philippjfr added this to the v0.10.2 milestone Nov 1, 2020
@philippjfr philippjfr added type: bug Something isn't correct or isn't working and removed TRIAGE Default label for untriaged issues labels Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't correct or isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants