rigetti / admittancemodels.jl Goto Github PK

I think the bot is now added so we can do this: @JuliaRegistrator register()

I'm not sure why input and output port matrices are allowed to differ for both Blackbox and PSOModel if there is only one vector of port names. Perhaps am I missing some obvious reason to do this?

The name is good in that the functionality is self-evident if you have read the paper, but for such a common operation it is a drag to type that many characters.

I suggest connect to indicate that the operation amounts to connecting multiple admittance models (or circuits) at their shared ports.

In discussing #11, Michael wrote:

After some thought, I agree that Cascade is not really necessary and given that things are generally more readable without it I'm in favor of making this change. I do like being able to specify what happens to the ports ahead of time though, as this would avoid some repeated code in the code for the paper. So if you have ideas for a better way to do that, that would be cool, though it's not that important.

To accomplish such functionality, it seems like a Port type would be necessary.

AdmittanceModels are not considered equal if columns of the input/output port matrices are permuted, with port names correspondingly permuted. I think it should be safe to consider such models equal---the ordering is entirely arbitrary and not even tied to gauge choice. Columns of the port matrix are not indexed directly, rather only after looking up the column index using the port names vector. Likewise, I think Circuits should be considered equal if equal up to a permutation of the vertices.

Furthermore, I noticed there is an isapprox method defined for AdmittanceModels. It appears convenient for some tests but I am not sure how correct it is to implement this because the atol and rtol flags are inaccessible. atol and rtol would be most useful if they were able to be specified for each matrix separately in the case of a PSOModel (or Circuit, which is not an AdmittanceModel but has a method similarly defined). As implemented, the method does not work in general, because isapprox is not defined for arrays of String port names, for example. Finally, I think there is a sense in which one could speak of different kinds of models approximating each other but that's not allowed here.

When C matrix has zero rows (such as when user accidentally defines resistive elements to disconnected ports) This matrix is not invertible. And in return julia returns a singularity error. Diagonal entries of this matrix (C should have a non zero diagonal if am I not mistaken) can be checked and user can be warned here.

... using PkgBenchmark.jl.

In src/circuit_components.jl, ports are assumed to be identified by Strings, but this not enforced elsewhere in the package. Probably the circuit components should have a type parameter for the port names.

See also #22; potentially one carefully thought out PR could address both issues.

The arguments of set_inductance!(c::Circuit, v0, v1, value) and related methods have an unfortunate ordering. Consider that for arrays, the syntax A[inds...] = X is lowered to setindex!(A, X, inds...), i.e. the set value precedes the indices.