Unpin sphinx and add networkx dependency.

[JoeZiminski]

This PR unpins the Sphinx version dependency from 5.1.1 as well as read the docs from 1.0.0. It also adds networkx which from the logs seems to be a dependency on the docs as an build warning was raised.

I can't detect any issues from the upgrade, although it's quite hard to test. I looked through the logs and they look similar, Number of warnings differs by 1. I went through every page and compared with current docs, everything looks the same and seems to have build without issue, but I did skim pages just randomly checking features like 1) links working 2) images rendering 3) generated values the same. The only differences I saw is that in the new version, the API docs typing is written a little neater using the new shortcuts (e.g. "|") and there is a slight stylistic change (the grey boxes around certain text is now gone). It would be great if someone could double check this in case I missed anything. I'm surprised it worked out the box 😅 seems too good to be true...

BTW I am getting a lot of warnings (~143) when building on Windows, does anyone else? If so we can look to address these when we look at further docs revisions. I am also getting random errors when building some of the sphinx galleries (for main also), it seems that when trying to delete some files they are still in use. I guess this is a platform filesystem difference issue, but would be good to hear if anyone else on windows has had this problem.

[zm711]

@JoeZiminski,

One of my goals is to work on the Windows friendliness, so I have an issue floating around about issues still present on Windows. Feel free to add the errors you're seeing there. Basically a lot of the cleanup during testing and doc building don't work on Windows due to permission differences between Windows and Unix-like systems. Alessio and I got the warnings down to 0 previously so it's wild that it's back to 143. Haha.

But I'm happy to look over the docs from this build later today :)

[chrishalcrow]

Hello, there seems to be some style inconsistencies on the API. A good example is the difference between plot_unit_waveforms_density_map and plot_unit_waveforms (https://spikeinterface--2861.org.readthedocs.build/en/2861/api.html#spikeinterface.widgets.plot_unit_waveforms_density_map) The first has white boxes, the second has grey. This doesn't happen in the current format (https://spikeinterface.readthedocs.io/en/latest/api.html#spikeinterface.widgets.plot_unit_waveforms_density_map)

I don't think this is a reason not to update.

I would guess it's to do with our docstrings not being up to scratch? More reason to join me here: https://github.com/SpikeInterface/SpikeInteface-Hackathon-Edinburgh-May24/issues/29 !!

EDIT: but everything else looks good. And the build on MacOS was fine.

[JoeZiminski]

That's great thanks @zm711, that explains why tests are not passing on my Windows machine 😅 Happy to help with this #2164! I hope you can't replicate the warnings and it is something to do with my setup!

Thanks a lot @chrishalcrow I just blindly assumed this was due to some RTD stylistic change but never actually checked. I think the reason is that for classes the parameters are in grey boxes but for functions the arguments are not. As far as I can tell, on the rtd-sphinx-theme website the example that is a function does also not have these grey boxes here (just above "5.2 C++ API"). In version 1.0.0 these examples are missing, they were introduced in 1.1.0. So my guess is that between 1.0.0 and 1.1.0 they have changed the styling to make a distinction between class and function parameters.

Thats great macOS build was okay, did you get warnings? BTW how do you generate the sharable link of your build, thats really cool!

[JoeZiminski]

BTW @zm711 I am having go mess with this due to a weird test failure, if you pull now there are 0 changes!

[zm711]

I'm actually going to ping @h-mayorquin too. That fact that you're testing with 0 changes and importing on ubuntu took 17 minutes is weird no?

[JoeZiminski]

Thanks that is strange, I pushed again but this is the run

[h-mayorquin]

Yes, so confusing why the import times are so long. They did not used to be like this but this is not exclusive to this PR:

Here are the times here https://github.com/SpikeInterface/spikeinterface/actions/runs/9111761651

And this is another PR:

https://github.com/SpikeInterface/spikeinterface/actions/runs/9104003224?pr=2858

I wonder when they became that bad for ubuntu and windows. As you can see, the culprits are comparison, curation and exporters.

[h-mayorquin]

E       AttributeError: module 'matplotlib.cm' has no attribute 'get_cmap'

We need to pin matplotlib, right now it is not clear that it should be high or low. I was not even aware that 3.0 is out! (damn : /).

Somebody wants to go and check with the release notes when the change was introduced? This should be a different PR issue as it is not related to the docs per se.

[JoeZiminski]

Thanks @h-mayorquin good catch! That was driving me insane, I could not figure out why adding one simple dependency (that is already installed in full) was causing the failure, but the change seems to trigger re-installing all packages rather than using some kind of cache.

Seems to be this change here, agree opening a new PR. Shall we try fixing that specific error and see if tests pass on 3.9.0?

[JoeZiminski]

Great I see @h-mayorquin has opened issue #2863. I've restored all changes on this PR and will wait for a decision over there and PR before tests pass. I think @zm711 if you wanted to test the new build it's still worth doing as this matplotlib test error should (?) not affect it.

[chrishalcrow]

Thanks a lot @chrishalcrow I just blindly assumed this was due to some RTD stylistic change but never actually checked. I think the reason is that for classes the parameters are in grey boxes but for functions the arguments are not. As far as I can tell, on the rtd-sphinx-theme website the example that is a function does also not have these grey boxes here (just above "5.2 C++ API"). In version 1.0.0 these examples are missing, they were introduced in 1.1.0. So my guess is that between 1.0.0 and 1.1.0 they have changed the styling to make a distinction between class and function parameters.

The problem is that both plot_unit_waveforms_density_map and plot_unit_waveforms are classes. But sphinx is treating plot_unit_waveforms_density_map as a function. I'm not sure why.

Thats great macOS build was okay, did you get warnings? BTW how do you generate the sharable link of your build, thats really cool!

Around 15 warnings, depending on whether networkx is there or not, which is roughly the same as running the old version.

It's actually your build! I believe that all pull requests auto-generate a docs build, hosted at e.g. https://spikeinterface--2861.org.readthedocs.build/en/2861/index.html

[JoeZiminski]

I think it is because SpikeInterface is doing a class-to-function conversion (e.g. used as function here and converted here). Because they are intended to be used as functions they are requested to be documented as functions here. So (I believe) that is the intended behaviour.

Great thanks, interesting about the warning number, I still get ~170 on macOS but they are all this warning which seems easy to solve. But I wonder if it's something with my setup I will see what others report!

It's actually your build! I

That's awesome! I'm going to copy this machinery for our repos! thanks