Feat/issue 303: de-duplicate plot spikes code

[AdityaBITMESRA]

I have created a pull request with the necessary instructions.

[sanjayankur31]

@AdityaBITMESRA : could you rebase your branch against development and update the PR to use development as the base branch too please?

[AdityaBITMESRA]

I have changed the base branch from master to development

[sanjayankur31]

I have changed the base branch from master to development

hrm, now we have conflicts again---did you create your feature branch from master or development? It should be off development and then we shouldn't have any conflicts :thinking:

[AdityaBITMESRA]

yes sir I think I created it against master branch...I will resolve this

[sanjayankur31]

No worries. Maybe a simple rebase will work:

git checkout feat/issue-303
git rebase development

that'll take your branch and "rebase" it on to development from master. It may not work given how master and development have now diverged, but worth a try. if your commits are small enough, you can even create a new feat/issue-303-1 branch from development and cherri-pick them?

More on both of these here:

• https://git-scm.com/book/en/v2/Git-Branching-Rebasing
• https://git-scm.com/docs/git-cherry-pick

[AdityaBITMESRA]

I think the conflicts are resolved. Can you verify sir @sanjayankur31

[AdityaBITMESRA]

Sure sir, sorry for the late reply. I will straightaway work on the instructions

[AdityaBITMESRA]

Hi sir @sanjayankur31 , I have updated the code with the necessary type hints and docstrings. Can you please review it?

[sanjayankur31]

Will do. I don't think we had any unit tests for the spike plotting bit either. Could you add some? See the unit tests for the time series plotting bit for examples.

[AdityaBITMESRA]

Okay sir

[AdityaBITMESRA]

Hello sir @sanjayankur31, I'm working on the test_plot_spikes_from_sonata_file test case in test_plot_spikes.py. It assumes a function called write_spike_data_to_hdf5 exists to write spike data to HDF5 in SONATA format, but I can't find its implementation. Could you clarify if this function exists and where it's defined? If not, how should I proceed with this test case?

[sanjayankur31]

Hello sir @sanjayankur31, I'm working on the test_plot_spikes_from_sonata_file test case in test_plot_spikes.py. It assumes a function called write_spike_data_to_hdf5 exists to write spike data to HDF5 in SONATA format, but I can't find its implementation. Could you clarify if this function exists and where it's defined? If not, how should I proceed with this test case?

I don't think we've implemented a function to write spike files to HDF5 format yet so write_spike_data_to_hdf5 doesn't exist. Maybe just add a test function but mark it to be skipped for the moment. I'll see if I can find a SONATA spikes file that we can use to test here.

[AdityaBITMESRA]

okay sir, thankyou for the help

[AdityaBITMESRA]

I have added test_PlotSpikes.py file @sanjayankur31

[sanjayankur31]

@all-contributors please add @AdityaBITMESRA for bug, ideas

[allcontributors[bot]]

@sanjayankur31

I've put up a pull request to add @AdityaBITMESRA! :tada:

[AdityaBITMESRA]

Thankyou sir for all the clear instructions, I will work on this asap

[sanjayankur31]

Is this ready for another round of review @AdityaBITMESRA ?

[AdityaBITMESRA]

yes sir @sanjayankur31 I have updated the code with the necessary instructions but it is failing the checks, so I am resolving them.

[sanjayankur31]

Sounds good. You should be able to run all the tests on your system locally using the test.sh script too. If only, unit tests, you can use pytest directly---and you can even tell pytest what specific test to run using the -k option. See pytest -h for usage instructions.

[AdityaBITMESRA]

Hi sir @sanjayankur31 , all checks have passed so I request you to review it once again if time permits

[sanjayankur31]

Thanks, I've just returned from leave, so I'll take a look this week.

[AdityaBITMESRA]

Sure sir...

[sanjayankur31]

I'm not sure it works properly. I ran the test_plot_spikes_from_data test and got this:

pytest -k test_plot_spikes_from_data

I don't see any spikes here. Is it working on your system @AdityaBITMESRA ?

[sanjayankur31]

the pynml-plotspikes command isn't working either from the looks of it:

$ cat KC_step_test.KC_pop.spikes 
0       0.14395000000004157
0       0.17676000000001174
0       0.2092299999999822
0       0.24151999999995283
0       0.27365999999992363
0       0.3056799999998945
0       0.3375699999998655
0       0.3693299999998366
0       0.4009399999998079
0       0.4323799999997793
0       0.46361999999975084
0       0.4945999999997227
0       0.5252799999996949
0       0.5555899999996673
0       0.5854499999996401
(ins)(neuroml-311-dev)[asinha@thor  pyNeuroML(feat/issue-303 *%=)]$ pynml-plotspikes KC_step_test.KC_pop.spikes 
Traceback (most recent call last):
  File "/home/asinha/.local/share/virtualenvs/neuroml-311-dev/bin/pynml-plotspikes", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/home/asinha/Documents/02_Code/00_mine/NeuroML/software/pyNeuroML/pyneuroml/plot/PlotSpikes.py", line 484, in main
    if args.lemsFile:
       ^^^^^^^^^^^^^
AttributeError: 'Namespace' object has no attribute 'lemsFile'

Can you please look into these? Please do test out the various functions one by one to make sure they work. The tests only check that a png file is created, not that it is a sensible plot :)

[AdityaBITMESRA]

Thankyou for looking into it sir @sanjayankur31. I will like to borrow some time so that I go through all the issues and correct them.

[AdityaBITMESRA]

Going through all the issues you pointed out ,they seem to be functional and can be solved if I take reference from PlotTimeSeries file, right?

[sanjayankur31]

Yes, you should be able to refer to plot-time-series. :+1:

[AdityaBITMESRA]

sir @sanjayankur31 I have updated the get_spike_data_files_from_lems function to add the necessary functionality and also removed the -lemsFile argument from the main function to align the logic with PlotTimeSeries function for lems file.

[AdityaBITMESRA]

I am really confused about not seeing any spikes after running the test_plot_spikes_from_data as pointed out earlier by you. I have tried to align with the logic of production plotspikes code but still not getting desired results. Is there anything which you would like to add?

[sanjayankur31]

I haven't had a chance to go over the code in detail yet. Maybe try with more neurons and more spikes---maybe there are too few to be able for us to see?

[AdityaBITMESRA]

sir @sanjayankur31 I increased the number of neuron and spike data in test file and this was the plot. What do you think about it?

[sanjayankur31]

Are the two populations supposed to overlap? Also why is the second population (-5001)?

What we want is for each cell to be a new row, so a new y value. So, if you have two populations, one with 0000 cells, and another with 5000 cells, we should see 15000 rows (y values), one per cell.

[sanjayankur31]

Maybe check your test data to ensure the cell ids are unique, and that each cell only belongs to one population

[AdityaBITMESRA]

I made the necessary changes. Please take a look sir @sanjayankur31

[sanjayankur31]

That looks correct now. so we're getting there. Please see why the tests are failing---once they pass I'll do another round of review.

[AdityaBITMESRA]

sure sir

[sanjayankur31]

The tests are failing with:

FAILED tests/plot/test_PlotSpikes.py::TestPlotSpikes::test_plot_spikes_from_files - ValueError: Image size of 876x620685 pixels is too large. It must be less than 2^16 in each direction

too much data in the test perhaps, or maybe see if you can reduce the DPI etc to reduce the size of the image.

Does it work when you run it locally?

[AdityaBITMESRA]

yes sir the plot was created but the test session was unusually very long and I ignored that it failed(1 failed,1 passed) because of the image size. I am resolving this

[AdityaBITMESRA]

Hello sir, @sanjayankur31 while going through the original PlotSpikes.py file which is in production, I found out that it had explicit error handling mechanisms using try-except blocks, which are missing in the refactored code.Should I work on reintroducing these mechanisms to ensure the code's reliability

[AdityaBITMESRA]

Hello sir @sanjayankur31, I have been trying to write the test function for test_plot_spikes_from_lems_files but I am encountering errors. I have taken the test_plot_time_series_from_lems_file as reference for writing. So am I right with my approach?

[sanjayankur31]

Hello sir, @sanjayankur31 while going through the original PlotSpikes.py file which is in production, I found out that it had explicit error handling mechanisms using try-except blocks, which are missing in the refactored code.Should I work on reintroducing these mechanisms to ensure the code's reliability

Yes---let's make it as reliable as we can

Hello sir @sanjayankur31, I have been trying to write the test function for test_plot_spikes_from_lems_files but I am encountering errors. I have taken the test_plot_time_series_from_lems_file as reference for writing. So am I right with my approach?

That sounds correct. What error are you seeing? Maybe we can help?

[AdityaBITMESRA]

Sir I got an index error (IndexError: list index out of range) and it is an error of the load_sim_data_from_lems_file function. Also can you tell me why we have not used plot_time_series_from_lems_file(plot_time_series_from_data_files was used) function in the lems test case of test_PlotTimeSeries function

[sanjayankur31]

Sir I got an index error (IndexError: list index out of range) and it is an error of the load_sim_data_from_lems_file function.

Right, can you paste your code and the exact error? You need to make sure that the lems file you're using does in fact have some files that are recording spikes listed in them using the EventOutputFile bit:

https://github.com/sanjayankur31/262670/blob/neuroml-ankur/NeuroML2/LEMS_KC_step_test.xml#L39

Also can you tell me why we have not used plot_time_series_from_lems_file(plot_time_series_from_data_files was used) function in the lems test case of test_PlotTimeSeries function

This looks like a mistake. it should be using lems_file there instead of trace_file. Thanks for pointing this out, I'll fix it now.

[AdityaBITMESRA]

import os import tempfile import numpy as np

def test_plot_spikes_from_lems_file(self): """Test plot_spikes_from_lems_file function""" spike_file = tempfile.NamedTemporaryFile(mode="w", delete=False, dir=".") for i in range(0, 1000): print(f"{i/1000}\t{np.random.default_rng().random()}\t{np.random.default_rng().random()}\t{np.random.default_rng().random()}", file=spike_file) spike_file.flush() spike_file_path = spike_file.name spike_file.close()

lems_contents = f"""

"""

lems_file = tempfile.NamedTemporaryFile(mode="w", delete=False, dir=".")
lems_file.write(lems_contents)
lems_file.flush()
lems_file_path = lems_file.name
lems_file.close()

pyplts.plot_spikes_from_lems_file(
    lems_file_name=lems_file_path,
    show_plots_already=False,
    save_spike_plot_to="spike-plot-from-lems-test.png",
)
print("plot_spikes_from_lems_file returned.")
assert os.path.isfile("spike-plot-from-lems-test.png")

os.unlink("spike-plot-from-lems-test.png")
os.unlink(lems_file_path)
os.unlink(spike_file_path)

I was using this lems file which seems to be wrong

[AdityaBITMESRA]

Thankyou for providing the lems example. I feel I can do it now

[sanjayankur31]

Yeh, check that the lems file is set up correctly. That should do it.

I fixed the test too: https://github.com/NeuroML/pyNeuroML/commit/a2066bcc94619c434d2c2aeb791e59d544a7f383

Do remember keep pulling the development branch into your branch here so that you have the latest code.

[AdityaBITMESRA]

yes sir I will keep this in my mind

[AdityaBITMESRA]

Hello sir @sanjayankur31 I was able to successfully run the test_plot_spikes_from_lems_file function and this was the plot created. Is this accurate?

[AdityaBITMESRA]

This was my lems file. I used the lems file from the plottimeseries and enhanced it with EventOutputFile bit