search

mne-tools / mne-connectivity

Connectivity algorithms that leverage the MNE-Python API.
https://mne.tools/mne-connectivity/dev/index.html
BSD 3-Clause "New" or "Revised" License
66 stars 34 forks source link

[MRG] [BUG] [ENH] [WIP] Bug fixes and enhancements for time-resolved spectral connectivity estimation #104

Closed ruuskas closed 1 year ago

ruuskas commented 1 year ago

PR Description

This PR closes #90, closes #84, and closes #17. Issues discussed in #70 and #73 are addressed. Please see the commit messages for description of the changes. Unfortunately, many fixes and enhancements are bundled together, as it's impossible to separate them without breaking functionality.

In summary,

The following ZIP archive contains a comparison between the current release version, the version of this PR, and the HyPyP implementation of spectral connectivity (mne_connectivity_testing_random_data.html). This shows that the issue with too high connectivity scores with random data is now fixed. The archive also contains a comparison with HyPyP using MNE-Python example data.

examples.zip

I marked this as WIP as I still have some concerns. Namely,

Merge checklist

Maintainer, please confirm the following before merging:

ruuskas commented 1 year ago

Reply to @adam2392

My understanding is that spectral_connectivity_epochs is that each Epoch is seen as a "sample" of connectivity, and then the difference is that the spectral connectivity either preserves the time-dimension, or not by estimating connectivity over time.

How does this differ from your proposal for spectral_connectivity_time? Is there a good reference that explains the differences? I suspect it would be nice to document this for users and our own sake to keep things straight in our head.

I apologize if this answer is too basic, but here's how I understand the difference between time-resolved connectivity and connectivity evaluated across trials.

My understanding is that when connectivity is computed over epochs (or trials), each epoch is seen as an independent realization of the same event, and the connectivity scores are computed as phase differences between the different trials at each time point. That is, the difference in phase is evaluated across trials. Therefore, the connectivity score measures how consistent the phase is across trials, and this type of connectivity estimation is suitable for evoked responses.

When computing time-resolved connectivity, the difference in phase is evaluated over time within each epoch. Hence, the connectivity score measures the consistency of the phase difference over time within an epoch. The scores for each epoch can then be averaged, or we could compute the standard deviation to assess dynamic connectivity. This type of connectivity estimation is useful for resting state measurements.

Equations 1 and 2 in Bruna et al., 2018 show the difference between connectivity over trials and time-resolved connectivity for the phase locking value. Mike X Cohen also covers the topic in this short YouTube video.

adam2392 commented 1 year ago

Thank you for your PR! I did not make it all the way through right now, but left a few comments and questions. My biggest question is: is this function trying to do too much? Can we split it in two functions, e.g. based on the different underlying methods used (Morlet wavelets, multitapers etc)? If I get it correctly, some of the parameters rule each other out right now? I wonder if the amount of parameters will be overwhelming for the user. What do you think, @adam2392 ?

Makes sense to break it up. Imo I ALWAYS get confused if I don't look at the function for awhile, so I think for a user, it must be worse. I think tho refactoring the code could be a downstream PR, where we possibly even introduce a deprecation cycle?

For now, based on https://github.com/mne-tools/mne-connectivity/pull/104#discussion_r960403420, we can move forward by just having sensible error checks/messages? How does that sound? @ruuskas and @britta-wstnr

ruuskas commented 1 year ago

Quote from @adam2392

Makes sense to break it up. Imo I ALWAYS get confused if I don't look at the function for awhile, so I think for a user, it must be worse. I think tho refactoring the code could be a downstream PR, where we possibly even introduce a deprecation cycle?

For now, based on https://github.com/mne-tools/mne-connectivity/pull/104#discussion_r960403420, we can move forward by just having sensible error checks/messages? How does that sound? @ruuskas and @britta-wstnr

I will add some error checks to the main spectral_connectivity_time function, and we'll see if it adds too much clutter/should be moved to a separate helper function.

ruuskas commented 1 year ago

Having done some testing with my real data, it is apparent that this method of computing the connectivity scores individually for each pair of signals is too slow to be practical. Using the aparc_sub parcellation, the computation time would be hours for a single connectivity method. I would suggest using a vectorized approach to calculation, similar to compute_sync in HyPyP. I would be ok with semi copy-pasting the code from HyPyP, it's licensed under BSD-3. Any thoughts, @adam2392 and others?

Also, I still don't quite get the spectral smoothing part or understand whether it's necessary. I think it's not used in spectral_connectivity_epochs.

adam2392 commented 1 year ago

I'm in favor of handling the speed issue in the next PR just to simplify reviewing. Is that okay with you? @ruuskas

Are you able to add unit tests based on the issues raised? E.g. a simulation that would fail before this PR but succeed with the new code?

adam2392 commented 1 year ago

Also, I still don't quite get the spectral smoothing part or understand whether it's necessary. I think it's not used in spectral_connectivity_epochs.

I think spectral smoothing is done and can be interpreted as a kernel method. My impression is that over epochs, the samples are "iid" but over time, the samples are dependent so the kernel can "change" this to "iid".

ruuskas commented 1 year ago

I'm in favor of handling the speed issue in the next PR just to simplify reviewing. Is that okay with you? @ruuskas

Are you able to add unit tests based on the issues raised? E.g. a simulation that would fail before this PR but succeed with the new code?

Could you clarify a bit? This is one of the issues, because I'm not sure how to verify the correctness of the connectivity computation. I do get values 1 with perfectly synchronized signals, but do not get 0 with random data. Every package seems to have a slightly different approach to computing the connectivity scores.

I'm ok with opening another PR, but for my MSc thesis project I will need to fix the speed issue ASAP, at least for my own use.

ruuskas commented 1 year ago

Also, I still don't quite get the spectral smoothing part or understand whether it's necessary. I think it's not used in spectral_connectivity_epochs.

I think spectral smoothing is done and can be interpreted as a kernel method. My impression is that over epochs, the samples are "iid" but over time, the samples are dependent so the kernel can "change" this to "iid".

I see, do you know of any publication containing proper justification for the smoothing ported over form frites?

adam2392 commented 1 year ago

Could you clarify a bit? This is one of the issues, because I'm not sure how to verify the correctness of the connectivity computation. I do get values 1 with perfectly synchronized signals, but do not get 0 with random data. Every package seems to have a slightly different approach to computing the connectivity scores.

In #90, #84, #70, #17 and #73 there seem to be some examples demonstrating the non-correctness of the current code base.

I would say test perfectly synchronized data. Then test random data. Then compare these to the code on main. Wdyt? @britta-wstnr any thoughts here on best test cases?

I'm ok with opening another PR, but for my MSc thesis project I will need to fix the speed issue ASAP, at least for my own use.

Sure then you can fix the speed issue here if it's easier. I might ask more questions tho cuz the changes will be larger :p. I would try to keep the changes small if possible so we can make sure each change is necessary. Also yeah semi copy/paste is fine if they have the same license. Just prolly comment in doc string where it came from.

ruuskas commented 1 year ago

I'm ok with opening another PR, but for my MSc thesis project I will need to fix the speed issue ASAP, at least for my own use.

Sure then you can fix the speed issue here if it's easier. I might ask more questions tho cuz the changes will be larger :p. I would try to keep the changes small if possible so we can make sure each change is necessary. Also yeah semi copy/paste is fine if they have the same license. Just prolly comment in doc string where it came from.

I would say the changes would be quite significant, since we have to do away with the for-loops (comprehensions) used to compute connectivity between signals and replace them with vectorized computation using np.einsum .

I just don't see much sense in testing my current modifications to the connectivity score functions if they need to be overhauled anyway.

ruuskas commented 1 year ago

In #90, #84, #70, #17 and #73 there seem to be some examples demonstrating the non-correctness of the current code base.

I would say test perfectly synchronized data. Then test random data. Then compare these to the code on main. Wdyt? @britta-wstnr any thoughts here on best test cases?

This is already done in the example notebook included in the opening message of this PR.

adam2392 commented 1 year ago

This is already done in the example notebook included in the opening message of this PR.

Ah yes. For automatic testing via pytest purposes, it would be great to have this as a unit test is what I meant. E.g. in mne_connectivity/spectral/tests/test_spectral.py. This helps us control against unintended changes. It would be helpful for when you refactor the code to vectorized operations because that refactor should only change the speed hypothetically, but not the end results.

I can help add some tests in a couple of days if that will help using your notebook as guidance.

In the meantime, feel free to implement the necessary vectorized changes and ping here if you need any discussions! Thanks for your work and discussion :).

Unrelated note: I see the CI is failing, I think that is not due to you, but changes in MNE-Python. I will fix those in #105

ruuskas commented 1 year ago

Ah yes. For automatic testing via pytest purposes, it would be great to have this as a unit test is what I meant. E.g. in mne_connectivity/spectral/tests/test_spectral.py. This helps us control against unintended changes. It would be helpful for when you refactor the code to vectorized operations because that refactor should only change the speed hypothetically, but not the end results.

I can help add some tests in a couple of days if that will help using your notebook as guidance.

I see, but assuming you mean assertion tests with the computed connectivity matrices, what should we test against to validate the correctness of the current implementation?

The fully synchronized case is clear since the resulting matrix will be full of ones, but what about random data. We cannot expect zero values for a short test example.

adam2392 commented 1 year ago

I see, but assuming you mean assertion tests with the computed connectivity matrices, what should we test against to validate the correctness of the current implementation?

I assume you mean current implementation in this PR. If you mean main branch code, then we don't need tests on that since that code is technically incorrect. I just meant to say the test should ideally show an error in main branch, but not error in your PR.

The fully synchronized case is clear since the resulting matrix will be full of ones, but what about random data. We cannot expect zero values for a short test example.

Some ideas: I think we can say assert all values are very small, or compare the matrix in norm to the synchronized case? Or run things multiple times as a Monte Carlo simulation and do some assertion on the average, but I guess this could potentially have long run time unless the dimensionality of the simulated data is very low.

ruuskas commented 1 year ago

I assume you mean current implementation in this PR. If you mean main branch code, then we don't need tests on that since that code is technically incorrect. I just meant to say the test should ideally show an error in main branch, but not error in your PR.

Yeah sorry, I meant the current implementation as in this PR. I can make a test case for that. How should the testing between two branches be handled? Is it ok to create a data matrix using the main branch and then load it in the test function?

Some ideas: I think we can say assert all values are very small, or compare the matrix in norm to the synchronized case? Or run things multiple times as a Monte Carlo simulation and do some assertion on the average, but I guess this could potentially have long run time unless the dimensionality of the simulated data is very low.

I see, but at least with my current example the values are not that close to zero, but still much smaller than scores computed using the main branch code. I'm afraid that any Monte Carlo simulation with a reasonable number of runs is out of question due to run time as you mention.

adam2392 commented 1 year ago

Yeah sorry, I meant the current implementation as in this PR. I can make a test case for that. How should the testing between two branches be handled? Is it ok to create a data matrix using the main branch and then load it in the test function?

Sorry, this is just meant as a framework. I would just implement the test as you expect it to work here, and then copy paste the code into main and confirm that it "fails" and then come back to this PR and confirm that it "works". That is good enough.

E.g. if I copy/paste the test you implement to main and run pytest, I would see a failure.

def test_spectral_connectivity_time_sim():
   ...

I see, but at least with my current example the values are not that close to zero, but still much smaller than scores computed using the main branch code. I'm afraid that any Monte Carlo simulation with a reasonable number of runs is out of question due to run time as you mention.

Perhaps what we do is:

Keep those tight for now and as long as you set random state rng = np.random.default_rng(seed) and then use rng.random... for the random functions, then this should be repeatable every time. WDYT?

ruuskas commented 1 year ago

Sorry, this is just meant as a framework. I would just implement the test as you expect it to work here, and then copy paste the code into main and confirm that it "fails" and then come back to this PR and confirm that it "works". That is good enough.

I see. I will make a test case.

Perhaps what we do is:

* compute the values for synchronized simulation

* compute values for non-synchronized simulation

* assert synchronized values are near 1

* assert non-synchronized values are < syncrhonized

* assert non-synchronized values are < some threshold(?)

Keep those tight for now and as long as you set random state rng = np.random.default_rng(seed) and then use rng.random... for the random functions, then this should be repeatable every time. WDYT?

Yeah, this makes it possible to compare the implementation in this PR to the main branch and any further modifications. However, it will not guarantee that the implementation is correct.

adam2392 commented 1 year ago

Yeah, this makes it possible to compare the implementation in this PR to the main branch and any further modifications. However, it will not guarantee that the implementation is correct.

Yeah it's unfortunate :/, but it'll be a great start! And it'll help you control for unintended changes as you improve the speed through your desired proposal of vectorizing the operations.

Thanks for your hard work! Lmk if you need any help / code-reviews as you move forward.

adam2392 commented 1 year ago

Hi @ruuskas just checking in to see if you need assistance?

ruuskas commented 1 year ago

Hi @ruuskas just checking in to see if you need assistance?

Hi! I spent some time getting the HyPyP style tensor multiplication approach to work and quickly realized it's way too memory hungry, at least for fine parcellations and 1000Hz data. I tried computing in blocks over the time axis and in the end realized that the for-loop style computation might actually be faster, at least for my data.

The two versions are now on two different branches so they can be compared. There's also a new test case with random data and fully synched data. Probably going to push tomorrow.

adam2392 commented 1 year ago

Nice! Looking forward to reviewing and seeing the results.

ruuskas commented 1 year ago

Nice! Looking forward to reviewing and seeing the results.

I pushed the changes now. The version utilizing numpy.einsum similarly to HyPyP is now found here.

Using einsum should in theory be faster, but in my case it results in an array with shape (n_epochs, n_tapers, n_channels, n_channels, n_freqs, n_times), or (78, 3, 460, 460, 21, 4000), which is a bit too large. I tried computing in blocks across epochs and time, which reduces the memory requirement but also introduces new for-loops (which we're trying to get rid off).

I will come up with some kind of a performance comparison and post here later.

ruuskas commented 1 year ago

I have been testing the two implementations, the one found in this PR, and the one on the branch spectral_con_einsum. It appears that for some peculiar reason, the matrix multiplication is actually quite slow.

To demonstrate this, I used a simple script to test on some real example data from MNE. Running with otherwise the same setup, the implementation in this PR finishes computation for one metric in about 10 seconds on my laptop. On the contrary, the implementation in spectral_con_einsum takes about 80 seconds.

Both runs were executed one epoch and all time points at a time, with the only difference being pairwise vs matrix multiplication computation of the spectral connectivity measures. Looking at profiling, most of the time in the matrix multiplication case seems to be spent computing the product between the analytical signal matrix and its complex conjugate (_multiply_conjugate_time).

Given the situation, we (at Aalto) are now looking at fixing the parallel computation in the implementation of this PR. For some reason, joblib doesn't work correctly and only slows the calculation down when n_jobs is set to larger numbers than one.

Here's the example script, should you also want to have a look. Based on MNE examples. speed_benchmark.zip

drammock commented 1 year ago

For some reason, joblib doesn't work correctly and only slows the calculation down when n_jobs is set to larger numbers than one.

it may be that the underlying linear algebra library used by numpy is already trying to take all available threads, so adding joblib parallelization creates competition for resources. Setting the environment variable OPENBLAS_NUM_THREADS (if Numpy is using OPENBLAS) to a small number can help diagnose this. See also https://stackoverflow.com/a/53224849 for other environment variables that might affect this (depending on your linalg library)

ruuskas commented 1 year ago

@drammock @adam2392 The computational speed issue with parallelization seems to have been due to memory mapping being disabled by default. With memory mapping enabled, I'm getting "good enough" run times for practical purposes.

The solution found by Simo Tuomisto and Thomas Pfau from Aalto Scientific Computing team is to set a reasonable value for MNE_MEMMAP_MIN_SIZE instead of the default None. Should we add this to the documentation of the function?

# Limit the minimum size of a disk-mapped array
export MNE_MEMMAP_MIN_SIZE=10M
# Set the MNE parallel cache location
export MNE_CACHE_DIR=/dev/shm

With joblib memory mapping enabled, the computational speed scales quite well with the number of available CPU cores.

Some clarification, courtesy of Simo Tuomisto:

While I was checking out this problem I added debugging statements into the functions and measured that the w-array was around 220MB. parallel_func has an argument max_nbytes (default: 'auto'):

max_nbytes : int, str, or None
Threshold on the minimum size of arrays passed to the workers that
triggers automated memory mapping. Can be an int in Bytes,
or a human-readable string, e.g., '1M' for 1 megabyte.
Use None to disable memmaping of large arrays. Use 'auto' to
use the value set using mne.set_memmap_min_size.

If one has not set the MNE_MEMMAP_MIN_SIZE, the limit is set to None.

    cache_dir = get_config('MNE_CACHE_DIR', None)
    if isinstance(max_nbytes, str) and max_nbytes == 'auto':
        max_nbytes = get_config('MNE_MEMMAP_MIN_SIZE', None)

This is passed to joblib.Parallel. By default, None will disable memmapping of large arrays:

max_nbytes int, str, or None, optional, 1M by default
Threshold on the size of arrays passed to the workers that triggers automated memory mapping in temp_folder. Can be an int in Bytes, or a human-readable string, e.g., ‘1M’ for 1 megabyte. Use None to disable memmapping of large arrays. Only active when backend=”loky” or “multiprocessing”.

Testing different values of MNE_MEMMAP_MIN_SIZE shows a sweet spot where if MNE_MEMMAP_MIN_SIZE is unset, one gets bad parallelization; if MNE_MEMMAP_MIN_SIZE=200M or less, one gets good parallelization and if MNE_MEMMAP_MIN_SIZE=300M or more, one gets bad parallelization. Thus I would guess that if the MNE_MEMMAP_MIN_SIZE is unset, no memory mapping is done and if MNE_MEMMAP_MIN_SIZE is set too large, the whole w array is memory mapped. Sweet spot is somewhere in the middle where either direct memory accesses are done or small pieces of the w array are memmapped. joblib.Parallel's own default is 1M.

Simo also noticed that the issue was already discussed in 2016.

drammock commented 1 year ago

The solution found by Simo Tuomisto and Thomas Pfau from Aalto Scientific Computing team is to set a reasonable value for MNE_MEMMAP_MIN_SIZE instead of the default None. Should we add this to the documentation of the function?

At a minimum we should add some documentation about this. It might need to go in multiple places (in both MNE-Python and MNE-connectivity). I'd also be interested to revisit the question about leaving the default as None... nowadays few of our users will be working with HDDs that spin (I think?) so setting a reasonable default like '1M' seems like a good idea to me at least. @larsoner @agramfort WDYT?

adam2392 commented 1 year ago

Yeah for connectivity, we should possibly document this in the Notes section for these functions perhaps? @ruuskas i think that would suffice.

Then for your explicit experiments verifying the speed up in the PR you can just set memmap.

As for default, let's see what rest of MNE Python team says.

Lmk when your PR is ready for review! Thanks for looking into this! Exciting that it works now better for you.

ruuskas commented 1 year ago

I will add a note to the documentation of the spectral_connectivity_time function.

I will try to clean up the code a bit before review, and also do some testing with simulated data we have acquired.

ruuskas commented 1 year ago

I have now marked this as ready for review @adam2392 @larsoner.

I think Hilbert transform should perhaps be added as a time-frequency decomposition method for connectivity estimation, but I will probably put that in another PR.

ruuskas commented 1 year ago

@adam2392 I noticed there were issues with pep style checks and fixed those.

ruuskas commented 1 year ago

While this PR is undergoing review, I have some other changes I'm planning for my own needs and might as well make a few PRs.

WDYT? @adam2392 and others.

ruuskas commented 1 year ago

Hi @adam2392, could you take a look whenever you have time?

adam2392 commented 1 year ago

Hey @ruuskas sorry for the delay. Been swamped. I will try to do a more comprehensive review tonight!

ruuskas commented 1 year ago

Hey @ruuskas sorry for the delay. Been swamped. I will try to do a more comprehensive review tonight!

Hi @adam2392. No worries, thanks! In the meanwhile, I've done four of the things I listed in an earlier comment, they're in the fork on separate branches for now.

  • [x] Adding corrected imaginary PLV as a connectivity metric

  • [x] Making the code more efficient by computing all spectral measures with a single run of the pairwise computation part. This should make the computation faster, as the most time-consuming step w[:, w_y] * np.conj(w[:, w_x]) would only be computed once instead of separately for each metric.

  • [x] Adding the possibility of specifying the frequencies of interest instead of frequency bands.

  • [x] (Perhaps) Adding filter-Hilbert as a method of obtaining the analytic signal.

  • [ ] (Perhaps) Removing the spectral smoothing, which would allow for a more efficient implementation of PLV (and ciPLV), using the algorithm presented in Bruna et al., 2018. This cannot be done if you want to keep the option for smoothing. I don't know if people use smoothing in general. Based on the tests in #73, smoothing seems to increase the ratio between the PLV of coupled signals and noise. However, those simulations were done with the earlier different implementation of PLV, whose interpretation is more complex.

ruuskas commented 1 year ago

Both this and the previous implementation are incorrect as far as my understanding goes. One could argue that the new implementation is more incorrect based on the tests I posted in #84.

adam2392 commented 1 year ago

Both this and the previous implementation are incorrect as far as my understanding goes. One could argue that the new implementation is more incorrect based on the tests I posted in #84.

Ahh... I see. So perhaps, let's figure out what's going on via #84 before this PR moves forward? You can always keep this PR open and open up a larger one whenever we converge in the GH discussion. WDYT?

ruuskas commented 1 year ago

Ahh... I see. So perhaps, let's figure out what's going on via #84 before this PR moves forward? You can always keep this PR open and open up a larger one whenever we converge in the GH discussion. WDYT?

Sure. We shall continue the discussion there.

ruuskas commented 1 year ago

Hi @adam2392. I made the changes here as well, so the CSD is now correctly aggregated with a weighted average and connectivity is computed thereafter. This closes #84.

ruuskas commented 1 year ago

Hey @ruuskas sorry for the delay. Been swamped. I will try to do a more comprehensive review tonight!

Hi @adam2392. No worries, thanks! In the meanwhile, I've done four of the things I listed in an earlier comment, they're in the fork on separate branches for now.

  • [x] Adding corrected imaginary PLV as a connectivity metric

  • [x] Making the code more efficient by computing all spectral measures with a single run of the pairwise computation part. This should make the computation faster, as the most time-consuming step w[:, w_y] * np.conj(w[:, w_x]) would only be computed once instead of separately for each metric.

  • [x] Adding the possibility of specifying the frequencies of interest instead of frequency bands.

  • [x] (Perhaps) Adding filter-Hilbert as a method of obtaining the analytic signal.

  • [ ] (Perhaps) Removing the spectral smoothing, which would allow for a more efficient implementation of PLV (and ciPLV), using the algorithm presented in Bruna et al., 2018. This cannot be done if you want to keep the option for smoothing. I don't know if people use smoothing in general. Based on the tests in #73, smoothing seems to increase the ratio between the PLV of coupled signals and noise. However, those simulations were done with the earlier different implementation of PLV, whose interpretation is more complex.

What do you think of this, do you want any of this to be merged into this module? The first three points are done and have been merged into the main branch in my fork. The 'hilbert' branch contains the fourth one although its otherwise outdated as I started to think that maybe Hilbert is not necessary for me.

I think it would be good to at least follow the suggestion from @britta-wstnr and @drammock and split the function into spectral_connectivity_time_multitaper, spectral_connectivity_time_morlet and potentially spectral_connectivity_time_hilbert.

If any of this is included, would it be best to put everything in separate PRs, or make another big one?

ruuskas commented 1 year ago

Nice work @ruuskas !

We just want to fix the CI stuff and add an entry to whats_new.rst now

Thanks @adam2392 ! I added the whats_new.rst entry though I'm not sure if it's good.

Notably, averaging over time when computing the metrics is strictly speaking not a bugfix, rather it is the behavior one would expect given the co-existing spectral_connectivity_epochs function (i.e., the result is static connectivity over time vs static connectivity over trials).

The time-resolved implementation found in frites.conn.conn_spec introduces a bias to the metrics and is harder to interpret (requires surrogate data which is not practical due to computational constraints). This was discussed in #73. Hence, I would suggest that we merge this PR as it is a more standard-ish way to implement resting-state connectivity analysis and corresponds to the formulations of the metrics in the relevant papers. If time-resolved connectivity is still wanted, it should probably be ported again since frites.conn.conn_spec has changed quite a bit after this function was originally ported over, although the implementation in this PR could also easily be changed to compute time-resolved connectivity. In any case such an approach isn't really suitable for all-to-all connectivity as connectivity matrices would have shape (n_epochs, n_channels, n_channels, n_freqs, n_times).

larsoner commented 1 year ago

CIs all better

ruuskas commented 1 year ago

CIs all better

Thanks @larsoner. Is it a bug that :func: and :class: are required for Sphinx?

larsoner commented 1 year ago

They are not strictly required, it's just more explicit. Without them the default_role kicks in, so if it ever changes, the ones without the :func: etc. would start failing. The real fix was the

``whatever``th

to

``whatever``-th

the non-alphanumeric made it clear that the inline code markup had ended

ruuskas commented 1 year ago

Thanks @adam2392. It will probably be easiest to combine the further updates into one PR again as there have been breaking changes here after they were initially implemented.

adam2392 commented 1 year ago

Okay I am merging this for now. Any issues, feel free to make a GH issue!

@ruuskas feel free to start the next PR! Will review whenever yiu need me to. Thanks for all the great contribution!