ENH:sparse.linalg: Add LaplacianNd class to special sparse arrays

Add a new special matrix, of the Laplacian, in multiple formats with a public API for testing and benchmarking.
Transferred from https://github.com/scipy/scipy/pull/18845 to split it into 3 smaller and focused pieces as suggested by reviewers.
Reference issue
Closes gh-18940 partially. To fully close gh-18940, we need to add some actual usage of the new lalacian API for testing and/or benchmarking, etc.
What does this implement/fix?
The Laplacian is sometimes called a "fruit fly" for experiments in scientific computing due its popularity and importance in applications. Adding Laplacian matrices in various formats with a public API as a LinearOperattor class for testing and benchmarking with a comprehensive dosctring documentation.
The proposed news matrix formats are similar to those being used in https://github.com/scipy/scipy/pull/18954 for Sakurai and Mikota matrices, which in that PR are located in a private module. If this and https://github.com/scipy/scipy/pull/18954 get merged, the next natural step is to move Sakurai and Mikota matrices to public API next to the Laplacian.

All checks pass, docstrung renders, search for "laplacian", ready for review!

@h-vetinari @tupui I have no expertise in API. It was not my intention to introduce a new API and I did not anticipate my use of the class becoming an obstacle for these PRs.
I am happy to edit the codes removing the class use and instead having an interface like  csgraph.laplacian(G, form="function") (which actually I have written myself some time ago). It's not as elegant as using the class but surely fine with me if consistency is more important. Should I?

@lobpcg if you add a function/method, it's also an API change and it also needs a discussion on the mailing list.
The round trip on the mailing list is meant to get folks attention and help to steer things in the right direction for SciPy.

open an issue with the design consideration

@h-vetinari I am unsure what the issue would be about - could you please clarify?

I have no expertise in API. It was not my intention to introduce a new API and I did not anticipate my use of the class becoming an obstacle for these PRs.

As I mentioned in that other thread, the current API for these special matrices is purely functional. As soon as you introduce classes, you need to think about what members these classes have, etc. All that creates a new kind of API (as opposed to "just" a new API in a style consistent with what's there already), which is what requires way more motivation & design.
If it's not essentially necessary (not counting performance & benchmarking, for example), I would suggest to stay with a functional interface. Otherwise:

am unsure what the issue would be about - could you please clarify?

Please reread this comment, which explains just that.

I think the main objection is the novel .callable interface. We already have LinearOperator objects to fulfill similar purposes. Is there something preventing implementing this feature by writing a laplacian() function that returns a LinearOperator (or subclass thereof) instance?

I think the main objection is the novel .callable interface. We already have LinearOperator objects to fulfill similar purposes. Is there something preventing implementing this feature by writing a laplacian() function that returns a LinearOperator (or subclass thereof) instance?

No. I am in process of rewriting it to make the API similar to that of the graph Laplacian.

Done, it's a function now. Ready for review!

I do think that it is good to have this return a LinearOperator rather than an explicit array. It's likely that it, and other such implicitly-defined special "matrices", ought to be exposed in scipy.sparse.linalg instead of scipy.linalg, though. After all, they are being introduced for use with the scipy.sparse algorithms. It doesn't currently have a _special_matrices module in it, but it certainly could/should.
To @h-vetinari's point, I do think this should only return a LinearOperator, unconditionally. Personally, I think it's fine to have a subclass of LinearOperator that also exposes additional functionality, like a @property that computes the eigenvalues on request or the explicit forms.

If that is not good enough for what you want to achieve, we need to talk about the design first, and not in a PR.

Eh, as long as the author doesn't mind rewriting, I'm not going to mind having a concrete example of the proposed design in front of me. As PR reviewers, we just need to be aware of what phase this is in and focus our review efforts on the design right now and not go line by line yet; that's on us. We ask that API-affecting proposals go to the mailing list first, but primarily so that people are alerted, not that design discussion must happen there and only there before any PR happens.

If that is not good enough for what you want to achieve, we need to talk about the design first, and not in a PR.

Eh, as long as the author doesn't mind rewriting, I'm not going to mind having a concrete example of the proposed design in front of me.

I appreciate your input, and you've been at this game longer than me. As far as I'm concerned, I have to disagree though. If I'm just looking at the PR, I'm have to guess what problems the implementation aims to solve, rather than having some high-level goals as a reference (where our combined efforts may well coax out a more elegant implementation, if only we knew what the constraints of the solution space are).

@h-vetinari the "form" is copy/pasted from https://github.com/scipy/scipy/blob/v1.11.1/scipy/sparse/csgraph/_laplacian.py#L12-L365

@rkern some calls of solvers require multiple formats and in different dtypes of the same matrix for efficiency. For examples, please see my benchmark codes in the sister PR  https://github.com/scipy/scipy/pull/18954 that additionally uses yet another format "banded" which is not always very efficient for the Laplacian in this PR, so is not implemented here.

If that is not good enough for what you want to achieve, we need to talk about the design first, and not in a PR.

Eh, as long as the author doesn't mind rewriting, I'm not going to mind having a concrete example of the proposed design in front of me.

I appreciate your input, and you've been at this game longer than me. As far as I'm concerned, I have to disagree though. If I'm just looking at the PR, I'm have to guess what problems the implementation aims to solve, rather than having some high-level goals as a reference (where our combined efforts may well coax out a more elegant implementation, if only we knew what the constraints of the solution space are).

The high level goals are illustrated in benchmark codes in the original PR, which got criticized for its format, style, and necessity so I guess nobody has looked at what the benchmarks are actually computing.

@h-vetinari the "form" is copy/pasted from https://github.com/scipy/scipy/blob/v1.11.1/scipy/sparse/csgraph/_laplacian.py#L12-L365

... which you added in afe1b6690d1b37a3adb88dec523806a6cf626cf8. I don't mind being outvoted here (it seems @mdhaber @tupui @perimosocordiae were fine with the variable output type), but it's something that we generally want to get away from.

The high level goals are illustrated in benchmark codes in the original PR, which got criticized for its format, style, and necessity so I guess nobody has looked at what the benchmarks are actually computing.

Already in the benchmark PR I had asked for a design rationale. For example, I cannot trivially answer from looking at your PRs: is the main usecase for this functionality the benchmarking? Or are we trying to solve some other usecases too? What are they? How are they done today, if at all? etc. Something like:
        mikota_pair_obj = mikota_pair(n)
        self.Ac = mikota_pair_obj.Kcallable
        self.Aa = mikota_pair_obj.Karray
        self.Bc = mikota_pair_obj.Mcallable
        self.Ba = mikota_pair_obj.Marray
        self.Ab = mikota_pair_obj.Kbanded

is not self-explanatory at all.

@rkern some calls of solvers require multiple formats and in different dtypes of the same matrix for efficiency.

That's why I said that the LinearOperator subclass that you return can expose those on demand. Changing the type of the returned object based on a parameter is a something that we've done in the past (you copied the scheme from one such), but don't want to keep doing in the future.

If I'm just looking at the PR, I'm have to guess what problems the implementation aims to solve, rather than having some high-level goals as a reference (where our combined efforts may well coax out a more elegant implementation, if only we knew what the constraints of the solution space are).

You can certainly ask questions if the PR description doesn't answer those questions, but to be honest, I found his description fine for this purpose. I'm not saying that we have to take the code or leave it. We're having the discussion that you want! It's no skin off my nose if there's code to talk about too; it's the author's time that might be wasted, not mine.

@h-vetinari "Add a new special matrix, of the Laplacian, in multiple formats with a public API for testing and benchmarking." If I add actual testing and benchmarking to this PR, the PR would get too big to review again - catch 22.
The other PR, https://github.com/scipy/scipy/pull/18954/files, has the benchmarking already there. One can read the docstring to see the meaning of the naming convention and examples of using different formats. The benchmark code there in "def time..." gives examples of the kind of use of different formats while calling a solver. These examples are not directly applicable to the Laplacian here, because they are based on "banded" format which is not generally efficient for the Laplacian, but may be some idea of why different formats and dtypes of the same matrix are needed for a single call of a solver, lobpcg in that PR.

I do think that it is good to have this return a LinearOperator rather than an explicit array. It's likely that it, and other such implicitly-defined special "matrices", ought to be exposed in scipy.sparse.linalg instead of scipy.linalg, though. After all, they are being introduced for use with the scipy.sparse algorithms. It doesn't currently have a _special_matrices module in it, but it certainly could/should.

This seems the best proposal to me. Ie: add a place for sparse special matrices that return LinearOperator s (or subclasses). You could then add methods or properties to the sub-class that give you the other formats.

@rkern
LinearOperator subclass that you return can expose those on demand.

I have written myself probably the last large enhancement of LinearOperator but do not recollect it having an ability to add custom on-demand things like eigenvalues and a banded (used in my other PR) matrix form.
The other thing is that I am unsure if a solver can just call a callable object - a pointer to the dot-matrix function - directly (lobpcg can) doing this same call via the LinearOperator construct is equally efficient, is it?

Changing the type of the returned object based on a parameter is a something that we've done in the past (you copied the scheme from one such), but don't want to keep doing in the future.

it is actually not yet implemented in this PR. I do not know how a dtype can be efficiently set without having a dtype parameter.

I have written myself probably the last large enhancement of LinearOperator but do not recollect it having an ability to add custom on-demand things like eigenvalues and a banded (used in my other PR) matrix form.

Subclass LinearOperator and add @propertys and methods, as appropriate.
class LaplacianND(LinearOperator):
    def __init__(self, shape, bc="N"):
        ...

    @property
    def eigenvalues(self):
        eigvals = ...
        return eigvals

    def as_banded(self):
        banded = ...
        return banded

The other thing is that I am unsure if a solver can just call a callable object - a pointer to the dot-matrix function - directly (lobpcg can) doing this same call via the LinearOperator construct is equally efficient, is it?

Should be.

Changing the type of the returned object based on a parameter is a something that we've done in the past (you copied the scheme from one such), but don't want to keep doing in the future.

it is actually not yet implemented in this PR. I do not know how a dtype can be efficiently set without having a dtype parameter.

We're not talking about the dtype. We're talking about returning different kinds of objects depending on the form and return_evals arguments. We want to avoid that pattern in the future.

This seems the best proposal to me. Ie: add a place in scipy.sparse.linalg for sparse special matrices that return LinearOperator s (or subclasses). You could then add methods or properties to the sub-class that give you the other formats.

I'll refine this proposal: Instead of functions returning instances of LinearOperator subclasses, let's just expose the LinearOperator subclasses directly. The functions aren't really necessary once we've committed to use LinearOperator subclasses. We'd have to document the subclasses anyways, so the function is just another moving part that's not really needed.

@rkern
class LaplacianND(LinearOperator): how is it different from what this PR has started with at https://github.com/scipy/scipy/pull/18963/commits/470c9eb859ea965604b131b80d193995eb2d6d50
I was criticized for using a class and had to change the class to a function after that commit.

I was criticized for using a class and had to change the class to a function after that commit.

Could you please try to avoid to frame it so strongly? There was no criticism per se, much less a demand that it absolutely has to be changed, only the observation that introducing a different API needs to satisfy a higher level of motivation and scrutiny.
Your classes in #18845 were also not reusing any existing infrastructure and had members that did not have a coherent design.
In short, basing this on LinearOperator is really not the same as what has been discussed previously.
As you notice I also don't speak for everyone here, so now that there's a clear(er) proposal with broader support, I suggest to work in that direction, rather than relitigate a previous & obsolete approach.

The main difference is that it's a LinearOperator, which is our standard infrastructure for implementing these matvec-function-defined linear operators, rather than a novel ad hoc interface.
SciPy is a big system with a lot of components. It's hard for everyone to always keep everything in our heads at once. Sometimes, when we are given fully-implemented concrete proposals to review, the immediate surrounding context of that implementation becomes more salient than other parts of SciPy, and that can sometimes obscure higher-level design issues that are really the root cause of what we think needs improvement. Instead, you sometimes get lower-level changes suggested that don't really solve the higher-level root cause. And if one chases each of these low-level suggestions, one will probably do and redo a bunch of work until the higher-level design issues are uncovered and consensus is reached. This is why @h-vetinari has (reasonably) been asking us to take a step back and have the higher-level design discussion. Until we reach some consensus at that higher level, continuing to follow each low-level suggestion will probably be frustrating. While I don't think it's necessarily wrong to have an explicit PR to discuss those issues, that is a risk you assume (me too! I can't really do design until my fingers have typed import numpy as np\n\n\nclass ...). We all have a part to play in drawing back to get the high-level design issues settled.
You initially proposed to place these implementations in scipy.linalg next to the other special matrices that are currently implemented as explicit arrays. This is not intrinsically an unreasonable proposal (those are indeed the most analogous existing things in our API), but the difference between what you needed to implement and the other things that it was next to made it clear that something needed to be changed. At first, one thing that seemed obvious was to make these look exactly like the other functions that it was next to. That was a low-level suggestion that seemed obvious because of the immediate context. I wasn't quite aware of everything that you needed, so I just assumed that a function returning a LinearOperator would have been fine. That was my mistake, and you have my apologies.
But now we have the opportunity to take a step back and look at all of the things that you need and all of scipy and figure out where things fit and what parts of scipy should be used. We have a need to expose several different special linear transformations, but unlike the ones that we implement in scipy.linalg, we need these to be defined in a few different sparse formats and also implicitly defined as matvec functions. We use these as benchmarks and tests for our scipy.sparse.linalg routines, and some may be generally useful for computation regardless.
Given that, I think there is growing consensus behind my proposal to implement subclasses of LinearOperator under scipy.sparse.linalg with additional methods or properties to compute the other representations or auxiliary information as needed. I don't think anyone else requires extra functions to return instances of those subclasses now they would live in scipy.sparse.linalg.

@rkern I need a (trivial) sample of the interface that you suggest, please, to seed my revision. It looks like it would be just a small editing of my original class at the end.

No apologies needed - some more coding is just extra fun for me, as soon as there is a happy ending.

First my apologies to @lobpcg that I made a comment then I disappeared from the other PR, work got way too busy and I'm trying to contain my GitHub time recently within a more disciplined schedule.
Two things that I'd like to mention as data points, Laplacians are indeed very useful problem data for testing sparse solvers and other sparse algos. We already have quite a bit of 1d and 2d examples in iterative tests.
Second is that these are kinda sorta indeed special matrices anyways hence it would be nice if we can have laplacian1d or laplacian2d as special matrices in scipy.linalg. In fact if you want I can quickly make it for you.
My minor comment is actually this line
   from scipy.sparse.linalg import LinearOperator

The reason for this is because of cyclic imports and to a much lesser extent build complexity. In the past, I've tried to reduce interdependence of scipy.linalg and scipy.sparse.linalg so that we can actually build things in a tree structure like _lib at the top then linalg, fft, in the next level and slowly growing the branches. And unfortunately there is clarkson_woodruff_transform standing in the way that we overlooked, still importing cscmatrix from sparse.
Anyways, the discussion has already ramped up so I don't want to inject myself into it but just to give a generic overview of why having both a special matrix in linalg and also in sparse.linalg would make sense.
It is up to us to expose sparse.linalg to public or just leave it as a testing convenience is still open but I'd suggest splitting the implementation into two if I have any vote on the matter. I will circle back to the benchmark PR for reviewing and again my apologies @lobpcg for the delay.

Second is that these are kinda sorta indeed special matrices anyways hence it would be nice if we can have laplacian1d or laplacian2d as special matrices in scipy.linalg. In fact if you want I can quickly make it for you.

Could you please indeed make a dummy sample code there but passing all the tests and with all attributes for me to edit? Or maybe just take all my functions and the docsting examples from this PR and write the whole thing, laplacianNd, yourself? I see no point writing separate 1d and 2d cases when I have the already working code for Nd.
There are 2 more sparse and also banded matrices to go there in my other PR: sakurai and a mikota pair. They need additional "banded" format and way to handle two matrices, the stiffness and the mass matrix in a single class. They currently are written as classes just not LinearOperator class.

Yes let me do that for dense version.

@lobpcg I took a stab at the dense version as the following, I changed a few minor things

there were too many temporary array creations because of repeated and destroyed intermediate krons. So I converted them to numpy ops.
The speed was getting too slow for large-ish arrays. This one is about 20x-30x faster since we use the same arrays. It's still not optimized but I think this is good enough.

Example runs
>>> %timeit laplacian_nd([3, 2, 4, 5], boundary_condition='periodic')
122 µs ± 312 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)

>>> %timeit laplacian_Nd([3, 2, 4, 5], bc='P', form='array')
3.43 ms ± 4.88 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

Here is the function
import numpy as np
from numpy.testing import assert_allclose

def laplacian_nd(dims, *, boundary_condition='neumann',  dtype=np.float64):
    """
    Laplacian docstring here

    Parameters
    ----------
    dims : tuple
        Tuple of integers that describes the dimensions of the problem.
    boundary_condition : string
        The type of boundary condition requested Laplacian should have. The
        possible values are 'neumann', 'dirichlet', and 'periodic'.
    dtype : type
        requested dtype of the array. The default is np.float64. The dtype
        must be big enough to hold the entries of the Laplacian e.g. boolean
        arrays cannot be used.

    Returns
    -------
    L : ndarray
        Resulting 2D Laplacian matrix.

    """
    N = np.prod(dims)
    L = np.zeros([N, N], dtype=dtype)
    # Scratch arrays
    L_i = np.empty_like(L)
    Ltemp = np.empty_like(L)

    for ind, dim in enumerate(dims):
        # Start zeroing out L_i
        L_i[:] = 0
        # Allocate the top left corner with the kernel of L_i
        # Einsum returns writable view of arrays
        np.einsum('ii->i', L_i[:dim, :dim])[:] = -2
        np.einsum('ii->i', L_i[:dim-1, 1:dim])[:] = 1
        np.einsum('ii->i', L_i[1:dim, :dim-1])[:] = 1

        if boundary_condition == "neumann":
            L_i[0, 0] = -1
            L_i[dim-1, dim-1] = -1
        elif boundary_condition in ("periodic", "dirichlet"):
            pass
        else:
            raise ValueError("Unknown boundary condition type "
                             f"'{boundary_condition}'.")

        if boundary_condition == "periodic":
            L_i[0, dim-1] += 1
            L_i[dim-1, 0] += 1

        # kron is too slow for large matrices hence the next two tricks
        # 1- kron(eye, mat) is block_diag(mat, mat, ...)
        # 2- kron(mat, eye) can be performed by 4d stride trick

        # 1-
        new_dim = dim
        # for block_diag we tile the top left portion on the diagonal
        if ind > 0:
            tiles = np.prod(dims[:ind])
            for j in range(1, tiles):
                L_i[j*dim:(j+1)*dim, j*dim:(j+1)*dim] = L_i[:dim, :dim]
                new_dim += dim
        # 2-
        # we need the keep L_i, but reset the array
        Ltemp[:new_dim, :new_dim] = L_i[:new_dim, :new_dim]
        tiles = int(np.prod(dims[ind+1:]))
        # Zero out the top left, the rest is already 0
        L_i[:new_dim, :new_dim] = 0
        idx = [x for x in range(tiles)]
        L_i.reshape(new_dim, tiles,
                    new_dim, tiles)[:, idx, :, idx] = Ltemp[:new_dim, :new_dim]
        L += L_i

    return L

I've run the following test for about 1000 times, and the results agree with your version.
rng = np.random.default_rng()

for _ in range(100):
    ndims = rng.integers(1, 5, 1).item()
    dims = rng.integers(2, 10, ndims)
    print(_, dims)
    assert_allclose(laplacian_nd(dims=dims, boundary_condition='dirichlet'),
                    laplacian_Nd(shape=dims, form='array', bc='D'))

I skipped the eigenvalues parts since that is not the pattern we have for special matrices but we can add them to the docstring notes for a quick reference.

Found a mistake when the dimensions are 1. Sorry about that random is not random enough. I'll fix it right away

@ilayn thanks, this is helpful but not the main obstacle. @rkern writes above "add a place in scipy.sparse.linalg for sparse special matrices.... there is growing consensus behind my proposal to implement subclasses of LinearOperator under scipy.sparse.linalg with additional methods or properties to compute the other representations or auxiliary information as needed. "
This is what I really need someone to help with: create a fully formally operational (passing all CI tests) analog of scipy/linalg/_special_matrices.py in scipy.sparse.linalg with one example that uses  subclasses of LinearOperator. An earlier version of this PR https://github.com/scipy/scipy/commit/470c9eb859ea965604b131b80d193995eb2d6d50 does use a class (generic, not LinearOperator) and may serve as an appropriate starting point for this if this example would be the Laplacian.

Ok I did the dense version here since this PR was doing ‛scipy.linalg‛. I'll do something for ‛sparse.linalg‛.

Ok I did the dense version here since this PR was doing ‛scipy.linalg‛. I'll do something for ‛sparse.linalg‛.

@ilayn An efficient dense version is still useful for sparse (mine was a just lazy implementation using sparse.toarray()) as well.
Please have a look at  https://github.com/scipy/scipy/commit/470c9eb859ea965604b131b80d193995eb2d6d50 before you start writing the new scipy/sparse/linalg/_special_matrices.py module. If you would like to do it within this PR should I do something on my side to enable your editing of it?

OK now I have read the whole thing a few more times above so I'll try to reproduce my understanding in some words. Please go over and correct me if I misunderstand it.

The reason why we had this PR is to eventually use it on the test and benchmark suite of scipy.sparse. Since scipy.linalg had special matrices already we first thought to include them here to later on use it on sparse side.
This led to the discussion regarding here not being the best place probably for such involved class disrupting the pattern for scipy.linalg._special_matrices.pt.
Now we have a dense version anyways https://github.com/scipy/scipy/pull/18963#issuecomment-1657181604 for scipy.linalg, it can go in with a few basic tests additions and this PR would be good to go.
Next step is to come up with a structure on the scipy.sparse.linalg side with a canonical implementation for such test arrays, using Laplacian as the first use-case. Then this structure can be reused for adding more sparse.linalg examples.

Is this sort of the right structure so far? Am I missing anything out?

I think it is nice to have it in linalg.special_matrices catalogue anyways.

Specifically, I'm happy enough to have a See Also note in this section of the scipy.linalg docstring to point to the scipy.sparse.linalg catalogue rather than have a separate dense-only implementation.

This PR is modifying linalg and not sparse.linalg so whatever we do will be happening over scipy.sparse anyways hence I'm not sure why we need to have this one. I'll open one for scipy.sparse.linalg separately since that is what has been requested. So I think I'm getting lost again. Closing a PR doesn't delete it anyways

I need a (trivial) sample of the interface that you suggest, please, to seed my revision.

Sure. I'm basing this on the last commit with the class version.
class LaplacianND(LinearOperator):
    def __init__(self, shape, bc="N", dtype="float"):
        self.bc = bc
        super().__init__(dtype, shape)

        # Precompute eigenvalues. This is fast enough, so we don't need an on-demand property.
        self.eigenvalues = ...

    def _matvec(self, x):
        ...  # Same as your `callable()`

    def tosparse(self):
        ...  # Same as your `lsm()`

    def toarray(self):
        return self.tosparse().toarray()

It might be worthwhile to implement _rmatvec() as well.

@lobpcg has agreed to move the code in this PR to scipy.sparse.linalg.

Yes and I am implementing it since @lobpcg asked me to do it. I don't know what is special about this PR to be honest. We can link it to this discussion too. So why the hassle to revert and do all that in their repo?

Nothing special about this PR. If you want to start a new one, please feel free.

Yes and I am implementing it since @lobpcg asked me to do it.

Ah, got it. Missed that request.

@rkern
    def toarray(self):
        return self.tosparse().toarray()

@ilayn has already came up with a direct efficient implementation replacing toarray
"It might be worthwhile to implement _rmatvec() as well." - The Laplacian is symmetric so typically would not need that.

@ilayn if/when you make a new PR, please consider adding me as a contributor?

@ilayn if/when you make a new PR, please consider adding me as a contributor?

Of course. I think now that you invited me I can revert the branch and add to your branch.

Tested for rebase priviledges. Just ignore the brief noise that I'll make here.

@rkern in https://github.com/scipy/scipy/pull/18963#issuecomment-1662310407 how to define  _matmat to be the same as  _matvec, please advise?
Adding after the _matvec definition
    def _matmat(self, x):
        return _matvec(self, x)

gives the error "NameError: name '_matvec' is not defined"
While @ilayn is writing the Laplacian class as LinearOperator here, I need to implement myself a similar construct in https://github.com/scipy/scipy/pull/18954

@ilayn I now know how to write the Laplacian as the LinearOperator class after finishing a similar edit in https://github.com/scipy/scipy/pull/18954/files
If you have not yet started actual editing, I'd be glad to do it myself quickly. I'll still need help, I guess, moving the code into the new module in sparse after that.

I need in _special_matrices
from scipy.sparse.linalg import LinearOperator
but it gives the error
ImportError: cannot import name 'LinearOperator' from partially initialized module 'scipy.sparse.linalg' (most likely due to a circular import)
probably due to
from ._special_matrices import *
in __init__.py
Does anyone know how to fix it?

It just needs to be moved to its new place under scipy/sparse/linalg

It just needs to be moved to its new place under scipy/sparse/linalg

Making a new module, e.g., _special_matrices, in  scipy/sparse/linalg requires some structure incorporating it with documentation etc. I need help with this.

Let me do that now for you. In the meantime, I'll move everything from linalg to sparse.linalg. In the meantime could you please conjure up conceptually a few basic tests for different boundary conditions? They don't need to be large arrays. But we should cover nonsensical entries like negative dims or whatever the pathological cases you might think of.

OK moved everything to a separate location in sparse. Also modified a few things here and there. Noticed a few things,

I think we should not use shape word here. It is very confusing since it is not the shape of the laplacian and hence I suggest we stick to dims. Because that the information it is carrying.
The code formatter you are using is a bit too aggressive on the array slices and expanding everything with arr[(a+b):(a+2*b)] becoming    arr[
         ( a + b ) : ( a + 2 * b )
        ]

I don't have any strong opinions about the line breaks and other details but I think this is a bit overzealous correction.
The variable names starting with underscore is a practice for marking them private variables. But we don't use that convention since everything is scoped inside the functions. Hence it increases the letter count and doesn't contribute to readability. We only use it when a word is too close to a reserved or common keyword.

I just noticed that I forgot to create a test file. I'll add that one

@ilayn thanks very much! Circle CI refguide  fails but easy to fix following their advice.
Docsting examples can be duplicated in the test.
Please feel free to edit as you like. My editing process is very inefficient. Some edits I do from my cellphone...
I used "shape" since it's the shape of the grid, but it is confusing indeed.
The default bc should be Neumann, like in the graph Laplacian.
I have no preference in or enough knowledge of good formatting and simply use block.

I think we should not use shape word here. It is very confusing since it is not the shape of the laplacian and hence I suggest we stick to dims. Because that the information it is carrying.

Agreed that it shouldn't be called just shape and the LinearOperator has a shape attribute that is the shape of the Laplacian matrix. Bikeshed: I don't quite like dims either, as it also sounds like something that might be referring to the Laplacian itself; maybe grid_shape or domain_shape?
Either way, the argument and the attribute ought to be named the same.

There is an issue with cirle-ci imports that gives an error in refguige which I do not have a clue how to fix. Could someone please have a look and advise?

Please advise how to fix this refguide error
=================== scipy.sparse.linalg =================== Non-deprecated objects in all: 34 Objects in refguide: 33 ERROR: objects in scipy.sparse.linalg.all but not in refguide:: LaplacianNd This issue can be fixed by adding these objects to the function listing in init.py for this module scipy.sparse.linalg.LaplacianNd

All current unit tests pass, so the basic functionality has been apparently achieved! Will add more tests for trivial things, fix the docsting, and improve the style as suggested. Thanks everyone for helping!

I think private vars are a bit overkill for such a simple class. Ideally LinearOperator should have been based on dataclasses but that's beyond the point hence we just need a consistent structure.
Hence in the functions or methods using bc=self.boundary_conditions is fine for my thinking. But self.bc or any other shorthand notation is pollution of namespace in my opinion

But self.bc or any other shorthand notation is pollution of namespace in my opinion

But the class already gets that member set in __init__, so how is the namespace not "already polluted"? That's part of my point actually - let's not pollute the member namespace (by making self._bc it private). And then don't pollute the function with pointless variables that could just as well read from self._bc directly (i.e. don't assign it to a temporary).

Hence in the functions or methods using bc=self.boundary_conditions is fine for convenience.

There is no self.boundary_conditions currently. I agree that that should be the name if we want to make the member public. But I actually don't think it's a good idea to make it public, because then it looks like it's user-settable even post instantiation, and that would pointlessly force us to carry around guards for wrong values of self.boundary_conditions in every function.

And if you do want to make it a public member, then the easiest way to avoid that it is settable is as follows:
@property
def boundary_conditions(self):
    return self._boundary_conditions  # only set in __init__

There is no self.boundary_conditions currently. I agree that that should be the name if we want to make the member public.

Indeed, this is what I meant; . namespace should be clean and unambiguous.
For the settable-ness, I typically go with the Python's "no private members, we are all adults" vibe. But yes @property fixes this nicely anyways so we can use it.

For the settable-ness, I typically go with the Python's "no private members, we are all adults" vibe.

I don't mind that, as long as we don't carry checks for boundary_conditions everywhere (so that adults who overwrite class members wrongly get to keep the pieces). But in this case I think the property would not be an undue maintenance burden to avoid that slight pitfall.

The discussion about the class setup is above my knowledge of python, so I hope that @ilayn  makes the changes.
If renaming is done, please also make the corresponding change in docsting and tests to keep everything green.
I have a question about the need of dtype here. Maybe just make all matrices in int8 and get rid of the dtype parameter?

Please advise: We should turn eigenvalues into a method rather than computing them in init? This way, one could request only a specific number (not all of them) of the smallest eigenvalues and we could also provide the corresponding eigenvectors? Providing all eigenvectors always would take lots of memory since they are dense float arrays.

Refguide gives no errors any longer, but instead of producing the documentation page leaves:
Sparse arrays with structure
..autosummary::
toctree:
generated/

at https://output.circle-artifacts.com/output/job/b22d3e2a-98d9-49c6-8cbe-5b2da2a9c679/artifacts/0/html/reference/sparse.linalg.html
I do not know how to fix it, so any help is greatly appreciated.

Refguide gives no errors any longer, but instead of producing the documentation page leaves:
Sparse arrays with structure
..autosummary::
toctree:
generated/

at https://output.circle-artifacts.com/output/job/b22d3e2a-98d9-49c6-8cbe-5b2da2a9c679/artifacts/0/html/reference/sparse.linalg.html
I do not know how to fix it, so any help is greatly appreciated.

I have pushed a commit to fix this for you

This is getting a bit complicated but I'll try to simplify and leave it as a working state. Then if needed we can add remove stuff; changes I'll do tomorrow if all agree with below.

Since this is an analog of scipy.linalg.special_matrices, instances of this class will not hold or modify its state hence let's stick to verbose and descriptive variables. This self.bc or self._bc won't do anyone good if they see it in auto-complete. Because that's just us cutting corners.
Also if somebody wants to fiddle with the internals, not our problem. Because we are not responsible from the state, we just need something that multiplies stuff and has structure. In fact that's why this should have been a dataclass anyways. If LinearOperator plays nice I can give it a go. Not a big fan of double inheritance but here since things are static I guess we are game.
Let's make the eigenvalues @lazymethod and only compute if requested. Denseness of 5000 eigs are not a problem but it is a problem if we are doing a python for loop for range(5000) just to get 3. Hence if we can't get the top ones then probably that closed formula is not that useful and can be left to the user.

Thoughts?

As far as attributes are concerned, I'm quite happy for the grid shape and the boundary conditions to be plain-old public attributes with no special mechanisms to prevent people from doing anything to them. Most I would do is document that they are to be provided at construction time and not to be reset later. Mixing in @dataclass would probably be overkill.
If these attributes are public, they should be named the same as the arguments that set them. So the arguments and the attributes would be named dims (though I still prefer grid_shape) and boundary_conditions.
As far as the eigenvalue computation is concerned, I'm happy to keep it in the __init__(). It's a loop over the number of dimensions of the domain grid, not the number of eigenvalues. At most, I'd move it to a @property.

The formula for the eigenvalues is vectorized so there is no long loop indeed, but we still compute them all as floats. Since the setup is matrix-free, the matrix size can easily be huge. A decade ago I used already 100x100x100 grid, i.e., 1M size for testing.
Selecting m smallest is tricky but I think that I can come up with a way to do it avoiding computing them all. However, we are nearly done with the rest in this pr, so maybe just keep them all as is and make a separate PR for m smallest, also providing the corresponding eigenvectors known analytically as well.

I have changed the default dtype from int 32 to int8 in the latest revision. After all the tests still pass, which appears to require a bit of massaging retyping the array to float before calling LAPACK eigensolver, I propose to just hardcode dtype=np.int8  and remove it from parameters. Using any other dtype larger than int8 is just a waste of memory. If the user needs it they can do it themselves.

I try to incorporate all suggestions in the latest revision, with self.boundary_conditions and self.grid_shape which I like better than dims. Assuming all tests pass, it would be ready for the final review.
I still propose to just hardcode dtype=np.int8 and remove it from parameters. Using any other dtype larger than int8 is just a waste of memory. If the user needs it they retype themselves.
The eigenvalues we may just can keep as is to finish this PR quickly and later make an improvement turning it into @property as proposed in https://github.com/scipy/scipy/pull/18963#issuecomment-1656543825 , adding the eigenvectors and allow selecting m smallest (or largest) in a different PR

hardcoded dtype=np.int8 and removed it from parameters, instead using self._dtype = np.int8

The memory should not be a problem. And int8 is almost never usable with anything else. If you want to use it to demonstrate things, at some point some function will upcast it. The dense version can handle it as a parameter and it should be an input parameter anyways to keep things float32 etc.

Let me do some changes and we review it once and get this in.

The memory should not be a problem. And int8 is almost never usable with anything else. If you want to use it to demonstrate things, at some point some function will upcast it. The dense version can handle it as a parameter and it should be an input parameter anyways to keep things float32 etc.

@ilayn int8 is 4x shorter and likely 2x faster to process than even int32 and much more compared to float32. To generate something that is natively int8 into any larger format is a just waste of memory and other compute resources, especially since the user can trivially recast it as needed. I show an example of this in the docsting.
The Laplacain is commonly used to test cases as large as the memory allows so I disagree that the memory is not a problem.
If you have some more time to spend on this PR, it would be nicer to turn lap.eigenvalues attribute into a callable property like lap.eigenpairs.eigenvalues(m) and   lap.eigenpairs.eigenvectors(m) producing, e.g., the m smallest eigenpairs for m>0, m largest ones if m<0 and all if m==0.

Laplacian might be int8 but if you want to do anything with it multiplying with stuff or eigenvalues etc. you must upcast it so that memory saving will only work if it stays as is. The reason we are adding this in that we can use it later for other things. Hence it won't make any difference. The sparsity is already quite high so the performance impact would only be felt if we have 1e6 by 1e6 or higher which is typically beyond SciPy scope anyways.
This is just to hold necessary data in some container and I think we should get this in without too much frills since we already spent quite some effort and it will soon start to bring diminishing returns.

To multiply, you do not need to upcast the whole matrix. Every element of the matrix gets upcasted as needed, without increasing the memory use. That is basically how my _matmat is written, I think. It preserves the dtype of the argument (I have added a unit test to check this) but the matrix entries are like int8 upcasted as needed. If you multiply by an int8 in matmat, the output is int8, so I believe that the matrix representations should follow this too.
Scipy LOBPCG easily runs on 1e6 by 1e6 (sparse or matrix-free) or higher and a typical starting point for testing it, e.g., computing 100 eigenvectors of a 1M size Laplacian. There is so much RAM nowadays easily available that the user can perform some crazy-large-size runs using scipy.
Anyway, I am reasonably happy with the current version functionally, and just want to make sure that the API is acceptable and the docstring is properly formatted. Please consult with me first if you would like to make changes that affect the functionality and feel free to edit for style/API.

Actually, one doesn't need to explicitly upcast from int8 even calling eigh, but the resulting eigenvalues are float32, failing my accuracy asserts tailored for float64, so I have to upcast the matrix to float64 to pass my tests.

eigh is upcasting it internally to the least larger dtype of float32, float64, complex64 and complex128 because it has to make LAPACK calls. So internally it is creating a copy of the array. But nevermind I made the dtype np.int8 because you can probably judge better but dtype is still a keyword for users who want to change it. I'll run the tests and push in a bit.

A previous revision has the dtype as a parameter with int8 as the default. How your implementation is different?

@ilayn I think that the code below correctly computes the m eigenvalues of typical interest (largest negative in our case) without computing them all. The idea is to compute first m eigenvalues in each of N directions, giving m*N values and select m from them.
 import numpy as np
from numpy.testing import assert_array_equal

m = 4
grid_shape = (4,10)
boundary_conditions = "Neumann"
boundary_conditions = "Dirichlet"
boundary_conditions == "Neumann"

# The original code
indices = np.indices(grid_shape)
l = np.zeros(grid_shape)

for j, n in zip(indices, grid_shape):
    if boundary_conditions == "Dirichlet":
        l += -4 * np.sin(np.pi * (j + 1) / (2 * (n + 1))) ** 2
    elif boundary_conditions == "Neumann":
        l += -4 * np.sin(np.pi * j / (2 * n)) ** 2
    else: # boundary_conditions == "Periodic"
        l += -4 * np.sin(np.pi * np.floor((j + 1) / 2) / n) ** 2

eigenvalues = np.sort(l.ravel())[-m:]

# The new code
grid_shape_min = min(grid_shape, tuple(np.ones_like(grid_shape) * m))

indices = np.indices(grid_shape_min)
l = np.zeros(grid_shape_min)

for j, n in zip(indices, grid_shape):
    if boundary_conditions == "Dirichlet":
        l += -4 * np.sin(np.pi * (j + 1) / (2 * (n + 1))) ** 2
    elif boundary_conditions == "Neumann":
        l += -4 * np.sin(np.pi * j / (2 * n)) ** 2
    else: # boundary_conditions == "Periodic"
        l += -4 * np.sin(np.pi * np.floor((j + 1) / 2) / n) ** 2

eigenvalues_min = np.sort(l.ravel())[-m:]

np.array_equal(eigenvalues, eigenvalues_min)

If this is right, it would also allow computing the corresponding m eigenvectors the same way - compute mN analytically corresponding to the mM eigenvalues and then select those eigenvectors that correspond to the m chosen eigenvalues.

For this PR, having the complete set of eigenvalues as an attribute is fine.
In a future PR, you can add an eigenvectors(k, largest=True) method that returns the largest (or smallest) k eigenvalues with their eigenvectors. I would not hold up this PR for that or remove the eigenvalues attribute to wait for that.

Code:

Renamed eigenvalues to eigs but I can revert. It felt really a bit too long everytime to test things out
added dtype as a keyword
made eigs a property
cleaned up the PEP8 problems

Tests:

I changed things a bit to remove the usage of platform dependent types
added explicit matmat test

Skipped the tests for now but let's get another round of reviews and push this in.
@lobpcg We need to get your system to compile and test SciPy, 50 commits for this PR is a bit on the excessive side for the load on the CI system.

@ilayn we also need https://github.com/scipy/scipy/pull/18963#discussion_r1286348304 which apparently got reverted.
I am unsure why you remove some dtypes from testing that worked fine on all tested CI platforms, most importantly int8. Could you please at least add  int8 back?
Let us please run the tests - I would sleep better knowing if nothing is broken...

I am unsure why you remove some dtypes from testing that passed fine on all tested CI platforms, most importantly int8. Could you please at least add int8 back?

int8 is already tested in all tests, and the reason why we don't use longs is because they are platform dependent, unsupported and cause only problems so far. Hence we switch to explicit bit length types. We really only use 32 and 64 realistically.

I notice missing https://github.com/scipy/scipy/pull/18963#discussion_r1286570845

It is a read-only property now.

The current revision is a bit of a mess, very unfortunately. The tests would surely fail, e.g., because of matmat, and even the refguide is now broken...

I've tested locally both on linux and windows and nothing fails. The refguide complains that now we return a sparse array instead of matrix. We are trying to get rid of sparse matrix usage hence the use of arrays. I'll fix that.

I am unsure how to proceed.

I propose that we review this and get it in. Too many changes are happening in every commit so having a review round and addressing things in chunks would be appreciated.

@ilayn "The refguide complains that now we return a sparse array instead of matrix. We are trying to get rid of sparse matrix usage hence the use of arrays."
What do you mean? The functionality has not changed and refguide passed as well as all the tests in my previous revision.

@ilayn "int8 is already tested in all tests, because it is the default dtype as you mentioned above"
Int8 is the default for the matrix. The test that you have changed has nothing to do with the matrix. Its dtype is for the matmat input argument, not for the matrix that is never constructed in this test.

What do you mean? The functionality has not changed and refguide passed as well as all the tests in my previous revision.

There is an ongoing transition from numpy.matrix based sparse arrays to numpy.array based sparse arrays. spdiags is still using np.matrix. As we continue with transition things will slowly start to change towards arrays. That was the ref-guide complaint and also I forgot the float64 upcasting.
Also if you check the details the resulting Laplacian was having larger nonzero elements because of the initial L=0 line now it should be reduced. And in fact because I forgot to fix the element numbers we should be able to see the difference in the refguide failures.

Could you please run all tests in CI and the circle ci with the fixed refguide? I would like to see everything green and read the correct documentation before checking the code again.

Int8 is the default for the matrix. The test that you have changed has nothing to do with the matrix. Its dtype is for the matmat input argument, not for the matrix that is never constructed in this test.

np.int8 was not in the original tests, np.int_ is np.int32, np.intc is np.int64, np.longlong is platform dependent 32 or 64bit integer.
I can add it to the types anyways if we are happy with the rest (a minor issue since np.int8 vs all other types are working).
But I would gently reiterate my request that let's review it thoroughly once, do the changes and then get it in. It went through 2 rewrites already and consuming a lot of maintainer attention.

I'm actually neutral on the computation being in the property. I think it's fast enough to be in the constructor, and if it's fast enough to be in the constructor, then I think it's fast enough to be done inside the property, too. If it's not fast enough for a property, then it shouldn't be in the constructor either and moved out to a method.
If we're not doing the computation in the property, I don't think we need a property at all. This isn't an attribute worth protecting. No one is going to even want to assign to it. grid_shape and boundary_conditions are more likely to be traps (but even those I wouldn't bother doing more than documentation).

In common language, the eigenvalues is a property of the Laplacian, not a method.
For the future enhancement, where only m selected eigenvalues and eigenvectors are compute, the number m needs to be passed as a parameter to the property.

So, I would prefer this being a property with computation to minimize later changes.

Properties can't receive arguments. If you want it to evolve later to receive arguments, it must be a plain method.

Don't worry about the "plain language" meanings of "property" and "method". These are technical terms used to distinguish two specific things in the Python language. They don't take on all of the connotations from their English usages. We use methods to model things like this all of the time; it's fine.

@ilayn I greatly appreciate you taking over!  I'll refrain from making any changes to the code myself now in this PR to simplify the final process.

@ilayn I greatly appreciate you taking over! I'll refrain from making any changes to the code myself now in this PR to simplify the final process.

No problem at all, but if you feel like it was an hostile takeover or something didn't feel right let me know and I can back off immediately. Sometimes, I'm not doing the full due disclosure in the heat of the moment.

We're getting some new build failures that appear to be unrelated. They appear to stem from having a complex type already defined when trying to typedef a new one in the SuperLU codebase. Is it related to #18975 or one of the numpy changes concerning complex types?

I would add np.int8 to
INT_DTYPES = [np.int32, np.int64]
in tests. Does not add much to the test_input_output_dtype_shape_consistency which tests the Laplacian operator, but is a new important possibility in  test_dot.
The name test_input_output_dtype_shape_consistency   should be changed to something line test_linearoperator_shape_dtype

We're getting some new build failures that appear to be unrelated. ...

I created an issue for this: https://github.com/scipy/scipy/issues/19036

@ilayn you use a variable dtype for arrays. It is still best to compute them entirely in int8 and upcast only on return, rather than actually running all their computations in dtype>int8 as happens now.

That would make copies at the end and would eat away the performance we gained from keeping it int8. So i don't think there would be a noticable performance difference.

That would make copies at the end and would eat away the performance we gained from keeping it int8. So i don't think there would be a noticable performance difference.
In the dense case we are reusing the array that we are going to return hence no copies are done except the scratch arrays.

Creating a copy of int8 to make float once on return is a negligible cost compared to running everything in float for no reason.

There is not enough data for that to matter. Here is a comparison
>>> %timeit LaplacianNd((3, 4, 5)).toarray().astype(np.float64)
110 µs ± 957 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)

>>> %timeit LaplacianNd((3, 4, 5), dtype=np.float64).toarray()
112 µs ± 650 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)

>>> %timeit LaplacianNd((30, 40, 5), dtype=np.float64).toarray()
333 ms ± 18.4 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

>>> %timeit LaplacianNd((30, 40, 5)).toarray().astype(np.float64)
136 ms ± 233 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

So it only matters around 6000 which is pretty much acceptable for such sizes. So there is not much difference. No difference sparse with sparse since sparsity structure is identical
>>> %timeit LaplacianNd((30, 40, 5)).tosparse().astype(np.float64)
3.02 ms ± 16.1 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

>>> %timeit LaplacianNd((30, 40, 5), dtype=np.float64).tosparse()
2.99 ms ± 11.6 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

If we want to accelerate this function we need to find a kron-free construction then it would make a huge difference. But also that's outside the scope of this PR

@ilayn thanks for checking!  For small sizes, the cost of setting up the LinearOperator dominates. Sparse computation is not yet optimized for int8, I guess.
I think it's worth doing, since it does give some improvement always in dense and does not deteriorate in sparse; and it makes the code future proof when int8 ops become fully optimized.

Array access and memory allocation is quite fast in NumPy. Barely matters for one unit or 8 unit of memory. Hence that would be a premature optimization. The bottleneck is the kronecker products. Here is again measurements. if you want to measure more things please do it locally.
%timeit LaplacianNd((30, 40, 5), dtype=np.float64).matvec(np.ones(6000))
537 µs ± 2.25 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

%timeit LaplacianNd((30, 40, 5), dtype=np.int8).matvec(np.ones(6000))
538 µs ± 3.32 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

%timeit LaplacianNd((30, 40, 5), dtype=np.int8).matvec(np.ones(6000, dtype=np.float64))
536 µs ± 2.85 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

%timeit LaplacianNd((30, 40, 5), dtype=np.float64).matvec(np.ones(6000, dtype=np.float64))
534 µs ± 2.33 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

I'd say let's get this in as is because like I mentioned this is just a sparse array and not a function to be used for all sparse arrays.

Dtype has no effect on dot and is not even used in the function matvec which is matrix free.

Yes so with measurements above; array creation is same, and arithmetic operations are the same hence current implementation int8/float64 is not making any difference. The bottleneck is kronecker products and the matvec operation because rolling axes and slicing is dominating any dtype difference.

There is some noticable improvement, the code becomes cleaner, it's a trivial change to make, and in fact it has already been  implemented in an earlier revision that you have reversed, so I don't understand your resistance.

There is some noticable improvement,

I don't follow. Where is it?

>>> %timeit LaplacianNd((30, 40, 5), dtype=np.float64).toarray()
333 ms ± 18.4 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

>>> %timeit LaplacianNd((30, 40, 5)).toarray().astype(np.float64)
136 ms ± 233 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

For 6000x6000 dense arrays that's not even relevant. This is a sparse array anyways so I don't quite agree what you think is important and code is definitely not clearer. So I also don't understand why you are insisting on this minor detail.
But in general, I rewrote this PR twice so I'll leave it to others to take the decision because I have to move on to other things.

For 6000x6000 dense arrays that's not even relevant. This is a sparse array anyways so I don't quite agree what you think is important and code is definitely not clearer. So I also don't understand why you are insisting on this minor detail. Because I already made dense array creation 20-30x faster
But in general, I rewrote this PR twice so I'll leave it to others to take the decision because I have to move on to other things.

I am a perfectionist, especially where an improvement can be trivially achieved. This code is also important to me to be used as a sample for more special sparse arrays coming to this module soon from https://github.com/scipy/scipy/pull/18954/files
I greatly appreciate your effort to make this PR nearly perfect! I am happy to make this trivial change myself without taking more of your time if you would not mind.

@ilayn Your benchmarks also motivate adding a couple more trivial asserts:
assert_array_equal(LaplacianNd(grid_shape, boundary_conditions=bc, dtype=dtype).toarray(), LaplacianNd(grid_shape, boundary_conditions=bc).toarray().astype(dtype))
assert_array_equal(LaplacianNd(grid_shape, boundary_conditions=bc, dtype=dtype).tosparse(), LaplacianNd(grid_shape, boundary_conditions=bc).tosparse().astype(dtype))

to test_linearoperator_shape_dtype. Again, I would be happy to add this myself as well.

Sure go ahead. Please keep PEP8 limitations in mind.

Sure go ahead. Please keep PEP8 limitations in mind.

Done. Looks all green as before. LGTM

Is everyone happy that this is good to go now?

LGTM on my end! Thanks, everyone!

Thanks all and thanks @lobpcg especially for your patience while the corrected api for this was being figured out

Thanks everybody for great teamwork leading to good quality and the fast merge!


Added Database Snippets | Issue : #110

@UTSAVS26 I added all the database snippets as in issue #110
added stuff :
SQL Database Snippets
- SQL Connection Manager
- Execute SQL Query
- Bulk Data Insertion
- Transaction Handling
- SQL Schema Creation
MongoDB Database Snippets
- MongoDB Connection Manager
- Insert Document
- Query Documents
- Update Document
- Manage MongoDB Indexes

@SpreadSheets600 where is the test cases?

@SpreadSheets600 where is the test cases?

Shi .... I left it .... In ... Wait up I will commit them

@UTSAVS26 done sir :)


install_serverless directly install already kafka

Signed-off-by: Matthias Wessendorf<EMAIL_ADDRESS>install_serverless directly installs kafka script, so no need to keep this func

/cherry-pick release-v1.3

/assign @pierDipi


Address bastion native client Issues and add proper error messages

This PR addresses the following bastion native client Issues

Add proper error message when --target-ip-address flag when IpConnect is disabled
Disallow Native Client connections on Non Standard SKUs and add try/except blocks around JSON decoding

This checklist is used to make sure that common guidelines for a pull request are followed.
Related command

General Guidelines

[x] Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
[ ] Have you run python scripts/ci/test_index.py -q locally? (pip install wheel==0.30.0 required)
[ ] My extension version conforms to the Extension version schema

For new extensions:

[ ] My extension description/summary conforms to the Extension Summary Guidelines.

About Extension Publish
There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update src/index.json automatically.
You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify src/index.json.

Thank you for your contribution! We will review the pull request and get back to you soon.

please record the test cases based on the latest azure-cli

Please fix CI issues


hdfs: add resources allocation management

Which issue(s) this PR fixes

Fixes #556
Additional comments

Agreements

[X] I hereby declare this contribution to be licensed under the Apache License Version 2.0, January 2004.
[X] I hereby agree to grant TOSIT a copyright license to use my contributions.

For me it is weird to force the Xms and Xmx to the same value, it will be too complex to have a variable for Xms and another for Xmx ?

yes, we can do as suggested but the idea was to force this good practice described above:
When a JVM process starts, it initially allocates the minimum heap size. If the need for memory arises at runtime, it can go back to the OS and allocate more, up to the maximum heap size.
Why do we recommend setting these to the same value?
Those incremental memory allocations can cause unpredictable runtime performance. Additionally, starting with a low minimum heap size could cause instability as the need for a larger heap grows over time. If it needs to grow to the maximum, but the OS doesn’t actually have that amount of memory available to hand out, then the process could be subject to OutOfMemoryError conditions, or possibly even the OOM killer stopping the whole process on Linux. Setting minimum and maximum to the same value eliminates these potential sources of instability.


CI is stuck running integration tests for almost two hours

Bug Description
Integration tests in the CI are failing to be completed; initially the bundle-integration-v2 job appears to be stuck and the only logs we can see are:
bundle-integration-v2: commands[0]> pytest -vv --tb=native -s --model kubeflow --bundle=./tests/integration/bundles/kfp_latest_edge.yaml.j2 /home/runner/work/kfp-operators/kfp-operators/tests/integration/test_kfp_functional_v2.py
============================= test session starts ==============================
platform linux -- Python 3.8.10, pytest-8.3.2, pluggy-1.5.0 -- /home/runner/work/kfp-operators/kfp-operators/.tox/bundle-integration-v2/bin/python
cachedir: .tox/bundle-integration-v2/.pytest_cache
rootdir: /home/runner/work/kfp-operators/kfp-operators
configfile: pyproject.toml
plugins: operator-0.35.0, asyncio-0.21.2, anyio-4.4.0
asyncio: mode=strict
collecting ... collected 6 items

After almost two hours, the job fails and we get to see more logs. See from L22.
Everything looks normal, the test cases are passing up until the  tests/integration/test_kfp_functional_v2.py::test_viz_server_healthcheck test case, which fails, BUT the logs from the failure do not indicate an error from kfp-operators or any other charm, what we see is:
  File "/home/runner/work/kfp-operators/kfp-operators/.tox/bundle-integration-v2/lib/python3.8/site-packages/juju/client/connection.py", line 649, in rpc
    await self._ws.send(outgoing)
  File "/home/runner/work/kfp-operators/kfp-operators/.tox/bundle-integration-v2/lib/python3.8/site-packages/websockets/legacy/protocol.py", line 635, in send
    await self.ensure_open()
  File "/home/runner/work/kfp-operators/kfp-operators/.tox/bundle-integration-v2/lib/python3.8/site-packages/websockets/legacy/protocol.py", line 948, in ensure_open
    raise self.connection_closed_exc()
websockets.exceptions.ConnectionClosedError: sent 1011 (internal error) keepalive ping timeout; no close frame received
websockets.exceptions.ConnectionClosedError: sent 1011 (internal error) keepalive ping timeout; no close frame received
------------------------------ Captured log call -------------------------------
WARNING  juju.client.connection:connection.py:654 RPC: Connection closed, reconnecting
WARNING  juju.client.connection:connection.py:654 RPC: Connection closed, reconnecting
ERROR    juju.client.connection:connection.py:662 RPC: Automatic reconnect failed
ERROR    asyncio:base_events.py:1707 Task exception was never retrieved

To Reproduce
So far only reproducible in the CI.

Submit a PR from main (HEAD is in e0dc929 atm)
Observe the bundle integration job
Usually only the v2 fails

Environment
CI environment
microk8s 1.29-strict/stable
juju 3.4 (3.4.4)
charmcraft latest/candidate (3.1.1)
Relevant Log Output
See description, but can also be found here
Additional Context
No response

I have tested the CI just running the v2 integration tests with the --no-build flag.
sg snap_microk8s -c "tox -e bundle-integration-${{ matrix.sdk }} -- --model kubeflow --bundle=./tests/integration/bundles/kfp_latest_edge.yaml.j2 --no-build"

This allows us to just see what's happening with the failing job (bundle-integration-v2) and at the same time, it confirmed that the issue was not at build time. With these changes, I could replicate the issue, and also observed the following:
  File "/home/runner/work/kfp-operators/kfp-operators/tests/integration/test_kfp_functional_v2.py", line 237, in test_viz_server_healthcheck
    status = await ops_test.model.get_status()
  File "/home/runner/work/kfp-operators/kfp-operators/.tox/bundle-integration-v2/lib/python3.8/site-packages/juju/model.py", line 2469, in get_status
    return await client_facade.FullStatus(patterns=filters)
  File "/home/runner/work/kfp-operators/kfp-operators/.tox/bundle-integration-v2/lib/python3.8/site-packages/juju/client/facade.py", line 486, in wrapper
    reply = await f(*args, **kwargs)
  File "/home/runner/work/kfp-operators/kfp-operators/.tox/bundle-integration-v2/lib/python3.8/site-packages/juju/client/_client6.py", line 1340, in FullStatus
    reply = await self.rpc(msg)
  File "/home/runner/work/kfp-operators/kfp-operators/.tox/bundle-integration-v2/lib/python3.8/site-packages/juju/client/facade.py", line 659, in rpc
    result = await self.connection.rpc(msg, encoder=TypeEncoder)
  File "/home/runner/work/kfp-operators/kfp-operators/.tox/bundle-integration-v2/lib/python3.8/site-packages/juju/client/connection.py", line 649, in rpc
    await self._ws.send(outgoing)
  File "/home/runner/work/kfp-operators/kfp-operators/.tox/bundle-integration-v2/lib/python3.8/site-packages/websockets/legacy/protocol.py", line 635, in send
    await self.ensure_open()
  File "/home/runner/work/kfp-operators/kfp-operators/.tox/bundle-integration-v2/lib/python3.8/site-packages/websockets/legacy/protocol.py", line 939, in ensure_open
    raise self.connection_closed_exc()
websockets.exceptions.ConnectionClosedError: sent 1011 (internal error) keepalive ping timeout; no close frame received

From logs L156 shows a call to status = await ops_test.model.get_status(). This comes from this line in the integration test file.
From logs L162 then we see a call to await self.rpc(msg)
In logs L166 we see how websockets is used await self._ws.send(outgoing). This comes from this line.
In connection.py, we can see the conditions under which the message WARNING  juju.client.connection:connection.py:654 RPC: Connection closed, reconnecting is logged (L648-663):

            try:
                await self._ws.send(outgoing)
                break
            except websockets.ConnectionClosed:
                if attempt == 2:
                    raise
                log.warning('RPC: Connection closed, reconnecting')
                # the reconnect has to be done in a separate task because,
                # if it is triggered by the pinger, then this RPC call will
                # be cancelled when the pinger is cancelled by the reconnect,
                # and we don't want the reconnect to be aborted halfway through
                await jasyncio.wait([self.reconnect()])
                if self.monitor.status != Monitor.CONNECTED:
                    # reconnect failed; abort and shutdown
                    log.error('RPC: Automatic reconnect failed')
                    raise

From the logs, in L171 we see that the websockets call failed websockets.exceptions.ConnectionClosedError: sent 1011 (internal error) keepalive ping timeout; no close frame received, which explains the error log from (4)

From the previous observation (https://github.com/canonical/kfp-operators/issues/549#issuecomment-2256490424), I tried two other things to avoid calling await ops_test.model.get_status() and avoid (potentially) the issue:

Use url = await ops_test.model.applications["kfp-viz"].units[0].get_public_address() instead.

As we can see from the CI logs, this is still causing an issue. Very similar issue from the one reported:
  File "/home/runner/work/kfp-operators/kfp-operators/tests/integration/test_kfp_functional_v2.py", line 239, in test_viz_server_healthcheck
    url = await ops_test.model.applications["kfp-viz"].units[0].get_public_address()
  File "/home/runner/work/kfp-operators/kfp-operators/.tox/bundle-integration-v2/lib/python3.8/site-packages/juju/unit.py", line 132, in get_public_address
    defResult = await app_facade.UnitsInfo(entities=[client.Entity(self.tag)])
  File "/home/runner/work/kfp-operators/kfp-operators/.tox/bundle-integration-v2/lib/python3.8/site-packages/juju/client/facade.py", line 486, in wrapper
    reply = await f(*args, **kwargs)
  File "/home/runner/work/kfp-operators/kfp-operators/.tox/bundle-integration-v2/lib/python3.8/site-packages/juju/client/_client19.py", line 1763, in UnitsInfo
    reply = await self.rpc(msg)
  File "/home/runner/work/kfp-operators/kfp-operators/.tox/bundle-integration-v2/lib/python3.8/site-packages/juju/client/facade.py", line 659, in rpc
    result = await self.connection.rpc(msg, encoder=TypeEncoder)
  File "/home/runner/work/kfp-operators/kfp-operators/.tox/bundle-integration-v2/lib/python3.8/site-packages/juju/client/connection.py", line 649, in rpc
    await self._ws.send(outgoing)
  File "/home/runner/work/kfp-operators/kfp-operators/.tox/bundle-integration-v2/lib/python3.8/site-packages/websockets/legacy/protocol.py", line 635, in send
    await self.ensure_open()
  File "/home/runner/work/kfp-operators/kfp-operators/.tox/bundle-integration-v2/lib/python3.8/site-packages/websockets/legacy/protocol.py", line 948, in ensure_open
    raise self.connection_closed_exc()
websockets.exceptions.ConnectionClosedError: sent 1011 (internal error) keepalive ping timeout; no close frame received
------------------------------ Captured log call -------------------------------
WARNING  juju.client.connection:connection.py:654 RPC: Connection closed, reconnecting

Use json.loads(sh.juju.status(format="json", no_color=True, model="kubeflow"))["applications"]["kfp-viz"]["units"]["kfp-viz/0"]["address"]

This way actually works. I assume it is because we are not calling anything that uses websockets under the hood (i.e. get_status or get_public_address).
PLEASE NOTE this does not explain why for v1 everything is passing and for v2 we get the error. For example, in this run, the exact same code that executes the integration tests is passing for v1, but v2 is failing.
Workaround
To help alleviate this issue and unblock the CI, I suggest we apply the following change:
 async def test_viz_server_healthcheck(ops_test: OpsTest):
     """Run a healthcheck on the server endpoint."""
-    # unit = ops_test.model.units.get("kfp-viz/0")
-    # url = unit.data["public-address"]
-    url = await ops_test.model.applications["kfp-viz"].units[0].get_public_address()
+    url = json.loads(sh.juju.status(format="json", no_color=True, model="kubeflow"))["applications"]["kfp-viz"]["units"]["kfp-viz/0"]["address"]

I have found https://github.com/juju/python-libjuju/issues/458, which details the same issue as the one we are hitting in this repository. After checking with the juju team, we can confirm it is the same.
https://github.com/juju/python-libjuju/issues/458#issuecomment-754220158 explains in more detail the reason(s) for this disconnection to happen.
The juju team has also confirmed this is a known issue and that for now there is a workaround of setting juju's MAX_FRAME_SIZE to something bigger than the default. From this conversation:

...maybe you could try incresing the max frame size here? Its default is 4MB, but I believe this can occasionally been too little if a lot is changing very fast in Juju, since juju communicates with pylibjuju by sending a delta of all the updates since the last checkin.

I'm not sure exactly what would cause it, but this is a known issue with pylibjuju. Pylibjuju has an internal state that it keeps in sync with the controller via the AllWatcher which sends deltas of the state. The juju CLI doesn't use this anymore because its a bit of an anti-pattern and work is currently underway to use alternatives in pylibjuju.

This workaround is being tested in #553.

Excellent debug work!!


Add setting to disable restoring last session

It's useful that atom can remember the files/folders you had open last time and then re-open them next time you start atom. I can't seem to figure out how to turn off this behavior though, I generally want to start clean each time I open atom.
Is there a config option to enable/disable this feature? Or could we implement a dialog box when closing atom that asks if you want it to remember what you had open?

:+1:
but maybe it would be better (popups at closing are annoying!) to implement a command  "close-without-saving-session" that we can bind to a keyboard shortcut

Is there a config option to enable/disable this feature?

As far as I know, there's no such option currently. /cc @maxbrunsfeld

Or could we implement a dialog box when closing atom that asks if you want it to remember what you had open?

That's an interesting idea as well. Personally, I think a config option might be less annoying than a dialog which pops up every time you close Atom. Or even a new command, as @andya9 suggested. :musical_keyboard:

Related to https://github.com/atom/atom/issues/6495
(:+1: for the command suggestion)
I think a config setting to enable / disable this might not be a bad idea – to be fair, Sublime has a config setting for their equivalent of this feature (hot_exit).

I also find this behaviour unneeded. It's not a bad thing to have, but I'd prefer it to start clean. Make it a setting, preferably. I shouldn't have to remember to type a specific command or keybbnd. if I want Atom to close the way I desire. :+1:

+1 for setting to disable

+1 I want to disable this feature.

I think this should be disable by default and "restore last session instead of open current folder" being a config opt-in, not an opt-out.

+1 for the previous behavior, "latest" should be an option.

Some other discussion of this happened here: https://github.com/atom/atom/issues/6873.

:+1:

+1, this should be made an option in the config files.

+1

:+1:  for an option to disable "restore session" globally

From @olmokramer:
You can put this in your init.coffee:
process.nextTick ->
atom.workspace.getPaneItems().forEach ->
atom.workspace.destroyActivePaneItem()
It closes all pane items after Atom started.
At: https://discuss.atom.io/t/dont-open-previously-opened-files-on-new-start/5959/8
Plase, implement this behaviour by default :'(

:+1: +1 for an opt-in feature. Pretty please.

Is it possible to hook in atom with a plugin before it loads the last session?
If yes someone could make a splash-screen plugin for this problem, is that possible?

An simpler workaround for Linux is to make application.json a symbolic link to /dev/null:
ln -sf /dev/null ~/.atom/storage/application.json

Thanks for the suggestion @albertgasset, but didn't work for me. Gave me an idea though, and this works for me instead:
Remove the storage dir, and replace with an empty file with same name.

:+1:

+1 on this feature.
Seems a tad presumptuous to expect everyone to want to re-open their old files.
This is supposed to be a basic lightweight text editor. Not a full blown IDE. So you need to either stop calling it a text editor everywhere on the atom website, or you need to stop adding massive scope creep features like session preservation to something that was never intended to be a heavyweight IDE in the first place.
I was about 2 seconds from abandoning atom altogether until I saw the suggestion to replace the storage folder with a file of the same name. That doesn't get me out of the fact that this issue has already wasted hours of my time. Meanwhile I struggle to imagine anyone with a clear vision for this product would add such a random feature and do such a half-assed job of implementation when it clearly does not fit with the concept of "lightweight" and is shoehorning every single user in a use case they may not be happy with. Kind of expected better from "the people behind github" so I dunno, maybe this is just the less talented people behind github working on atom. What else could explain the lack of foresight in adding such a divisive and heavyweight feature without making it configurable?

stop adding massive scope creep features like session preservation to something that was never intended to be a heavyweight IDE in the first place

The most common requests for the session restoration feature were from users of TextMate and Sublime Text. Both of these programs have this behavior, and they are not IDEs; they are text editors. If you want to open a specific directory, and not the directories that you had opened previously, simply type this:
atom .

It's lightweight!

That said, we're not opposed to adding a configuration setting for this. As usual, we'd love to merge a PR adding this feature, if it works and has good tests.

+1

albertgasset and code-tree provided working work-arounds but unfortunately, it seems that if you use their method, Atom no longer remembers the "Toggle Tree View" setting which means that I would have to manually toggle the tree view every time I launch Atom in order to hide the side pane that displays the directory tree. Another work-around to disabling the last session restoration without breaking Atom's ability to remember the "Toggle Tree View" setting is to modify the permissions so that ~/.atom/storage/application.json is in read-only mode.

If we're going to add this, I'd love to start with a config-file-only setting rather than adding a checkbox to core settings. I think that there should be a high bar to adding more settings controls for usability reasons, and shipping it without UI first & doing some analytics on uptake seems like a good way to be cautious in cases like this.

+1 Would like to be able to toggle this behaviour.
I'm frequently working with a package that adds a custom menu pane to Atom, and this causes significant loadup overhead if I had it open when I last closed Atom. It takes up to 5-6 seconds to load, so I would prefer to have no tabs open when I start Atom.

I have different issue with application.json.
It seems like this file is constantly being updated. At least whenever I switch between Atom windows.
What about updating this file only when there is a change i.e. when you open/close Atom window, or when you close whole Atom editor?

Softlinking /dev/null to ~/.atom/storage/application.json gives me exactly the behavior I want: no previous file and a new blank text file named "Untitled".

@jenkstom Nice workaround!
Atom is still pretty slow to start.. always opening several windows makes this worse. I work on many different projects in a day and prefer a clean slate when I start atom. Hoping this becomes a setting eventually.

:+1: for adding that setting!
And thanks @jenkstom for the workaround!

@jenkstom What is the command you used?

mv ~/.atom/storage/application.json ~/.atom/storage/application.json.backup
ln -s /dev/null ~/.atom/storage/application.json

@jenkstom @albertgasset
Thanks for the tip, I've implemented it now :)
If I want to restore the original state, I only need to do this right?
~/.atom/storage/application.json mv ~/.atom/storage/application.json.backup ~/.atom/storage/application.json

Yes, that should put it back. Or you can just delete the soft link to save state on the next session.

Thanks. File permission works for me.

+1 here.. strongly dislike having random projects open when i start the ediotr

+1 to make this configurable

+1 would love to see this option
I have my atom config synced through dropbox across a windows desktop and a osx laptop, and of course the path to the last opened file is going to be different across the 2 platforms, so its really annoying to have atom try to open C:/Users/..... on my mac

I've gone through the code to investigate what can be done to implement this feature. There is an AtomEnvironment instance variable enablePersistence that controls saving current session's data. Unfortunately, it is also passed to Config class, where it controls Atom storing / reading user settings. The latter appears to have been added by @nathansobo in dc44d11a2cee008a0b1ff4ed6784420e125cd4fc.
@nathansobo, could you give some guidance on how you'd expect this feature to be implemented?

@freeatnet See this in-progress PR: https://github.com/atom/atom/pull/9042

@maxbrunsfeld Awesome, thank you!

+1 This causes unwanted issues and delays with virtual directories.

This was implemented in #11324/#9042 and will be out in Atom 1.8.0-beta0.

Could this please be reopened? As I was disappointed to discover the following:

This only applies to opening Atom without a path argument.

:(
I don't see the rational for this. If people don't want session restored, then they don't want session restored...
(still appreciate the partial implementation though, so thanks)

Also, you now lose your editor contents on close (even if session restore disabled, Atom does not ask me to save untitled editors).

I am using Atom 1.9.9 and do not see this feature. Please reopen.

Same here

There was a comment a while ago about not needing a UI for this config yet. You can add the setting to your config.cson
core:
    restorePreviousWindowsOnStart: false

Working for me on 1.10

@tschf Thank you, startup is now much faster for me!

@tschf Thanks, that works for me. So much better for me...

@maxbrunsfeld Do you think we could add this to the schema so people see it in settings?

Combine that with rewording theOpen Empty Editor On Start setting. https://github.com/atom/atom/issues/10623

@tschf I added that setting. It works for me when opening Atom without specifying a folder. But when I open Atom in a folder (atom some/dir) I still see the open files from the previous session. Since I almost never open Atom without specifying a folder, the issue remains for me

The issue remains, but @tschf comment stands and worked for me. I had a problem where I opened a webdav file (atom took a very long time to launch, and then kept crashing), so editing config.cson seemed to me at least a very minor price to pay.

I opened bootstrap.css.map. and now every time i open Atom, it opens it automatically and freezes.
this feature should really be able to turn off!

Use --clear-window-state for the time being.

Don't forget about the restorePreviousWindowsOnStart option (which is why this issue has been closed).

+1,  this annoys me a lot...

@martinosorb can you please elaborate?  What annoys you a lot?  If the lack of a setting to disable restoring the last session is what is annoying you, please be aware that the restorePreviousWindowsOnStart option will prevent restoring the last session.

That seems to work only in the case of atom being opened without a path specification, am I wrong?

@martinosorb Ah, good point.  Would you mind opening a new issue for that if one doesn't already exist?

Somebody above was proposing to reopen this one because of that issue.

I think it's best to open a new issue so as to not confuse the two (no setting to disable restoring state vs. setting not applying when explicitly giving a path argument).

Just in case
https://discuss.atom.io/t/remember-open-projects-when-reopening-atom/7053/37


colorfulSidebar crashes causing infinite login loop

See the video for this behavior:  https://streamable.com/s/9k8on/kuwkpa
When colorfulSidebar is enabled, it crashes and kicks the user out to the login screen.  Upon logging back in, it crashes again and kicks them back out again.
MacForge version 0.12.4.  colorfulSidebar version 1.7.0.
This doesn't happen with colorfulSidebar 1.6.0.
You have to reboot into Safe Mode and remove the plugin from /Library/Application Support and then you're able to login again

Well now. That seems really serious. Do you have crash logs for loginwindow at /Library/Logs/DiagnosticReports

Just put out 1.7.1 which should fix the issue. Please let us know if you’re still experiencing problems after updating.

1.7.1 fixed the issue! Thanks for the quick fix!

I've got the same issue with the last version on 10.13.6


Self-registration links should have stricter format requirements

Description
Self-registration links (e.g., https://simplereport.gov/register/this-link-here) are currently allowed to be any format so long as they're unique. We don't want this to be so open once organizations can make their own links.
Proposed solution
We likely want to limit these to alphanumeric/kebab case (e.g., shady-oaks) or maybe allow slashes as well (shady-oaks/foo-facility).

[ ] Enforce at GraphQL layer
[ ] Enforce at DB layer

I don't think we need this since we are deciding the reg links for orgs and facilities going forward. @nickscialli-usds feel free to re-open if you disagree.


wav2vecU missing import

🐛 Bug
No module named 'speechproc' when running wav2vecU vads.py script inside colab
Traceback (most recent call last):
File "scripts/vads.py", line 10, in
import speechproc
ModuleNotFoundError: No module named 'speechproc'
To Reproduce
Inside a colab that has cloned the repo and downloaded common voice.
https://colab.research.google.com/drive/1JEHyCaeI5TKdOO9VEJ0RyRH_wCpjadPm?usp=sharing

!python scripts/vads.py /content/cv-corpus-6.1-2020-12-11/sv-SE/train.tsv train.vads

Traceback (most recent call last):
File "scripts/vads.py", line 10, in
import speechproc
ModuleNotFoundError: No module named 'speechproc'

Expected behavior
Expects the script to run and make a train.vads file
Environment

fairseq Version (master):
PyTorch Version (1.8.1+cu101)
OS (Linux, colab):
How you installed fairseq (source):
Build command you used (git clone):
Python version: 3.7.10
CUDA/cuDNN version: colab setup
GPU models and configuration: colab setup
Any other relevant information: colab setup

Additional context
Running inside google colab

I resolved it by downloading VADfast that was needed in order to use speechproc.
!git clone https://github.com/zhenghuatan/rVADfast


Updating readme file

Starting to address #44.

Hi @veronicagg, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Veronica Giaudrone). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

@salameer please take a look and see if it addresses what you wanted as initial documentation. @amarzavery, it'd be great if you could review it too.

Hi Veronica, I think it's really good start but I would suggest putting the introduction info that you added on top of the installation instructions :)


Fix missing bracket

Summary
Fix missing bracket for JP document.
Requirements (place an x in each [ ])

[x] I've read and understood the Contributing Guidelines and have done my best effort to follow them.
[x] I've read and agree to the Code of Conduct.

Hi @risto24, thanks for fixing this issue! Once the CI builds pass, we'll merge your contribution 🎉


Update framework to 8.15.3

About this PR
📦 Updates org.elasticsearch.test:framework from 8.15.2 to 8.15.3
📜 GitHub Release Notes - Version Diff
Usage
✅ Please merge!
I'll automatically update this PR to resolve conflicts as long as you don't change it yourself.
If you'd like to skip this version, you can just close this PR. If you have any feedback, just mention me in the comments below.
Configure Scala Steward for your repository with a .scala-steward.conf file.
Have a fantastic day writing Scala!

🔍 Files still referring to the old version number
The following files still refer to the old version number (8.15.2).
You might want to review and update them manually.
project/Dependencies.scala

⚙ Adjust future updates
Add this to your .scala-steward.conf file to ignore future updates of this dependency:
updates.ignore = [ { groupId = "org.elasticsearch.test", artifactId = "framework" } ]

Or, add this to slow down future updates of this dependency:
dependencyOverrides = [{
  pullRequests = { frequency = "30 days" },
  dependency = { groupId = "org.elasticsearch.test", artifactId = "framework" }
}]

labels: library-update, early-semver-patch, semver-spec-patch, old-version-remains, commit-count:1

Superseded by #408.


fix broken links

Fixed broken links, updated some others and removed VS version.
Rick, the link https://www.asp.net/learn I changed to https://docs.microsoft.com/aspnet/overview, because that link is about ASP.NET.
/cc @Rick-Anderson @scottaddie @dend
Internal review URL

@mairaw @nokura Assuming everything is setup properly, index/TOC should be handed off daily for all of our projects; Nobuko can confirm.


The color of CMYK pixel format pictures is incorrect

https://github.com/mono/libgdiplus/blob/0477843237ea7168d345bfe7f9ee2f13cd315130/src/jpegcodec.c#L452
if (cinfo.saw_Adobe_marker) {
	b = (k * c) / 255;
	g = (k * m) / 255;
	r = (k * y) / 255;
}  else {
	b = (255 - k) * (255 - c) / 255;
	g = (255 - k) * (255 - m) / 255;
	r = (255 - k) * (255 - y) / 255;
}

set_pixel_bgra(lineptr, 0, b, g, r, 0xff);

It seems that the values of b and r are incorrect.
Red is assigned to b, and blue is assigned to r.
This leads to the color of CMYK pixel format pictures is incorrect.
I think the correct code should be:
if (cinfo.saw_Adobe_marker) {
	r = (k * c) / 255;
	g = (k * m) / 255;
	b = (k * y) / 255;
}  else {
	r = (255 - k) * (255 - c) / 255;
	g = (255 - k) * (255 - m) / 255;
	b = (255 - k) * (255 - y) / 255;
}

set_pixel_bgra(lineptr, 0, b, g, r, 0xff);

The calculations look correct to me based on the color theory. However, it is quite possible that the channels are swapped elsewhere in the code. What version of the library are you using? It would help a lot to have some  code/data to replicate the bug.

The calculations look correct to me based on the color theory. However, it is quite possible that the channels are swapped elsewhere in the code. What version of the library are you using? It would help a lot to have some code/data to replicate the bug.

Thank you for your reply.
The version of libgdiplus we are using is 6.0.4.
OS: ubuntu 20.04
Framework: .NET 5
I don't think the current color conversion is correct. For example, red:
C = 0%
M = 100%
Y = 100%
K = 0%
According to the current formulas:
b = (255 - k) * (255 - c) / 255;
g = (255 - k) * (255 - m) / 255;
r = (255 - k) * (255 - y) / 255;

R = 255 * 0 / 255 = 0
G = 255 * 0 / 255 = 0
B = 255 * 255 / 255 = 255
This will get the RGB value of blue instead of red.
This is also the same as the problem encountered in our production environment. The red in the picture changed to blue after repainting.
We tried to swap the positions of b and r and recompile, and this problem was solved.

I have to admit that you are probably right :-) Would you mind submitting a pull request with the change?


[Release] Bumped datadog_checks_dev version to 29.0.0

[Release] Bumped datadog_checks_dev version to 29.0.0
[Release] Update metadata

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

[ ] Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
[ ] Changelog entries must be created for modifications to shipped code
[ ] Add the qa/skip-qa label if the PR doesn't need to be tested during QA.

Is the check PR job failure expected?

Is the check PR job failure expected?

Sounds like a bug

Also there are 3 test failures: scylla, SNMP (unrelated to this change) and TCP on linux

Already reported on Slack.


Implementation of SoundPool on insert and update event.

Is your feature request related to a problem? Please describe.
Adding SoundPool  , to play a very short ringtone whenever the user saves of updates his/her Poem and notes. Thus providing improvized user experience by the application.
Describe the solution you'd like
Please assign this issue to me on GSSOC'23.Thank you.
Describe alternatives you've considered
No response
Additional context
No response
Code of Conduct

[X] I agree to follow this project's Code of Conduct

not required


Separate transparent background

Generally, Android has transparent icons and iOS does not. It would be nice to optionally add a transparent version of the icon.
Example:
icon.png <-- main
icon-transparent.png <-- transparent version
or perhaps
icon-ios.png
icon-android.png

Thanks for the idea.
To avoid duplicates, let's continue the discussion in issue #19.


Questions in get partial aggregate bonded transactions and remove the only cosigner from cosignatories list

1. An empty array was returned after  the request of partial aggregate bonded transactions
In sdk v13.0.0 , AccountHttp.aggregateBondedTransactions() seems not work , I test the url '.../account/{publicKey}/transactions/partial' ,it also returns null array .
The comment in code shows that the method AccountHttp.aggregateBondedTransactions() and the url '.../account/{publicKey}/transactions/partial'  show returns partial transactions.
2.The only cosigner in cosignatories list can not remove itself to turn a multisig  account into a normal account
It's OK while 2 or more cosigners in cosignatories list, but the problems occured while the last cosigner want to remove itself from cosignatories list, the node return Failure_Multisig_Modify_Min_Setting_Out_Of_Range  error.

@rg911 please verify if this is maybe a REST problem then tag roger ?

@BloodyRookie @Jaguar0625 @gimer

I don't see the partial transactions neither, will report to rest to have a look. @Vektrat


Move docker base image from Xenial to Bionic

Got a problem of building RPi release, revert this PR.


[5.x] Don’t save fields with save set to false

Need to re-open #336.
New use case is that I need a field to be not saved and editable in the CP. Using visibility:computed means that the field is not editable in the CP.

Still need to update the tests as they reference don't_save which is not the correct config field handle

Still need to update the tests as they reference don't_save which is not the correct config field handle

No? dont_save is just the handle of the field in the blueprint we're using for testing. The setting itself is called save.


Fix controller PHPStan errors

because symfony/security was missing as dev dependency
Something weird: symfony/security v5.x was not tagged.
https://packagist.org/packages/symfony/security

Strangely enough the Github Action did run on that commit, but Github simply doesn't show it!
https://github.com/rvanlaak/SettingsBundle/commit/18fc6900d5a808c00a29212006f3203755eaac9e/checks?check_suite_id=357203280


[5.7] Update CacheManager::driver @return directive

The driver method only calls the store method which return type is \Illuminate\Contracts\Cache\Repository, so this should be the same one.

Thanks.