Integrate raster plot

[ssgier]

Issue Number: #362

Objective of pull request: Integrate raster plot

Pull request checklist

Your PR fulfills the following requirements:

• Issue created that explains the change and why it's needed
• Tests are part of the PR (for bug fixes / features)
• Docs reviewed and added / updated if needed (for bug fixes / features)
• PR conforms to Coding Conventions
• PR applys BSD 3-clause or LGPL2.1+ Licenses to all code files
• Lint (flakeheaven lint src/lava tests/) and (bandit -r src/lava/.) pass locally
• Build tests (pytest) passes locally

Pull request type

Please check your PR type:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no api changes)
• Build related changes
• Documentation changes
• Other (please describe):

What is the current behavior?

• Code to generate raster plot inlined in tutorial notebook

What is the new behavior?

• Code to generate raster plot exposed in a new module: lava.utils.plots

Does this introduce a breaking change?

• Yes
• No

Supplemental information

• A few tests fail locally, but they do as well on main. I made sure the same subset of tests are failing and that they are unrelated to my change. The last cause is sqlite3.OperationalError: unable to open database file. I am running pytest with Python 3.10.9 under Linux. Is this issue known? If not, perhaps best to discuss separately. I will also take a look into what might be the root cause.
• I did not re-evaluate the notebook to make the review easier. However, I suggest that I rerun it eventually, for consistency.
• The assert pasted in the issue asserts that the stride is less than the number of time steps. But the way I read it, it should be less than the number of neurons. I changed it accordingly. Let me know if I misunderstood this. Also, I replaced the assert with raising a ValueError to placate the linter.
• I am currently not including a unit test because the new method just creates a plot. Let me know if it should still be tested in some way.

[mathisrichter]

Hi @ssgier
Thanks for your first contribution to Lava! I'm glad to see that you dug right in and created a PR. This is a good first start; the code that we had pasted in the issue was just meant as a starting point. I believe we could make this more user friendly by exposing more parameters to the user, making the function more flexible. I would also suggest to add input validation to catch errors early and provide meaningful feedback to users when they make a mistake. This should then be unit tested to ensure that the validation works as expected.

[mathisrichter]

Regarding your notes:

A few tests fail locally, but they do as well on main. I made sure the same subset of tests are failing and that they are unrelated to my change. The last cause is sqlite3.OperationalError: unable to open database file. I am running pytest with Python 3.10.9 under Linux. Is this issue known? If not, perhaps best to discuss separately. I will also take a look into what might be the root cause.

I am not aware of this issue. Could you look through the issues in Github to see if this has been filed? If not, please create a separate issue. This could be an environment problem.

I did not re-evaluate the notebook to make the review easier. However, I suggest that I rerun it eventually, for consistency.

Agreed. Re-evaluate it once you think the notebook iteself no longer needs to be changed.

The assert pasted in the issue asserts that the stride is less than the number of time steps. But the way I read it, it should be less than the number of neurons. I changed it accordingly. Let me know if I misunderstood this. Also, I replaced the assert with raising a ValueError to placate the linter.

Yes, I believe you are correct. Good catch. :)

I am currently not including a unit test because the new method just creates a plot. Let me know if it should still be tested in some way.

Best practice is to write tests for every qualitatively different behavior of a piece of code.
Things to test here are, for instance:

• that a figure object is returned with default parameters
• that a meaningful error is raised for all the invalid inputs you are handling (I suggested adding more)
• that no errors are raised when parameters are valid

[mathisrichter]

Looks very nice, @ssgier !
I remember you still wanted to reevaluate the Jupyter notebook? We can merge the PR once that is done.

[ssgier]

Following up on the locally failed unit tests: I had a look and there were actually two separate root causes:

1. On my system, the ulimit suggested in the documentation (4096) was not high enough. Pytest opened over 11K files during the run, a lot of them shared memory files.
2. The mnist tutorial test attempted to load a weights file which is stored with Git LFS.

Setting a much higher ulimit and installing Git LFS resolved the issues and made all tests pass. I am not running an officially supported Linux distribution, so I'm not sure if the problems can also arise with the supported OSes.

[mathisrichter]

Thanks for following up on that.

The first has been a problem for a while - this is due to a problem with the multiprocessing library we are using, I believe. We are working on exchanging that, which will hopefully resolve that issue.

The second could just be missing documentation about having to use Git LFS to get the files. Feel free to add that to the documentation where you think it would be appropriate.

[mathisrichter]

You PR is now merged. Thanks again for your work on this! We very much hope you will continue to support Lava with new features and bug fixes soon! :)