Add comments to network analysis file by TorkelE · Pull Request #906 · SciML/Catalyst.jl

TorkelE · 2024-06-03T21:39:05Z

I have gone through the network analysis file and added comments where appropriate. I am not super familiar with it, so I basically went through ti from start to finish, and where there were stuff that was not obvious, I tried to figure out what was going on and added a comment (the file is generally mostly readable without comments though, so not super much added).

The only changes that are not comments are a 4 instances of variable renaming for clarity (I think all should be fairly non-controversial, but am happy to change any back):

One case where there were some inconsistencies how the variables in a function was named.
One case where where we used sts as name (which makes less sense now when states is no longer used as a term in MTK. Since these corresponded to species I renamed it sps).
One case where a function I wrote used variable names inconsistent with the rest of the package.
One line where I thought the names where non-descriptive. You will have to judge this one when you see it.

In all cases, I have marked these.

TorkelE · 2024-06-03T21:40:56Z

src/network_analysis.jl

+    # Loops through each sub-graph of connected reaction complexes. For each, create a 
+    # new `ReactionSystem` model and pushes it to the subnetworks vector.
    for i in 1:length(lcs)
-        reacs, specs, newps = subnetworkmapping(lcs[i], rxs, complextorxsmap, p)


Here, I found it weird that rxs was used for the old reactions, and reacs was used for the new reactions (while newps and p was used for the new/old parameters). In this, and the previous subnetworkmapping function, I propose we change to newrxs, newspecs, and newps for the new reactions, species, and parameters.

TorkelE · 2024-06-03T21:41:22Z

src/network_analysis.jl

    end
-    rxs, specs, newps   # reactions and species involved in reactions of subnetwork
+
+    newrxs, newspecs, newps


New names, used in this and the next function, see it for clarity.

TorkelE · 2024-06-03T21:42:12Z

src/network_analysis.jl

+    newrxs = allrxs[rxinds]
+
+    # Find the species that are part of the sub-reaction network.
+    specset = Set(s for rx in newrxs for s in rx.substrates if !isconstant(s))


Mark as new line. It is not, it has simply been pushed down one step and uses newrxs instead of rxs.

TorkelE · 2024-06-03T21:44:11Z

src/network_analysis.jl

 function conservationlaw_errorcheck(rs, pre_varmap)
    vars_with_vals = Set(p[1] for p in pre_varmap)
-    any(s -> s in vars_with_vals, species(rs)) && return
+    any(sp -> sp in vars_with_vals, species(rs)) && return


We don't really use s as much throughout, changed this to sp which is more commonly used (and also more clearly a species).

TorkelE · 2024-06-03T21:45:16Z

src/network_analysis.jl

+    # Retrieves nullity (the number of conservation laws). `r` is the rank of the netstoichmat.
    nullity = size(N, 1)
-    r = numspecies(rn) - nullity     # rank of the netstoichmat
-    sts = species(rn)


sts kind of suggests states, which is no longer a thing. I would change to us, but since these are species specifically, I changed this (here and in two calls over the next few lines) to sps.

TorkelE · 2024-06-03T21:47:12Z

src/network_analysis.jl

@@ -122,6 +136,7 @@ function reactioncomplexes(::Type{SparseMatrixCSC{Int, Int}}, rn::ReactionSystem
    complexes, B


Here, and in a lot more cases, I wonder if there are more descriptive names for the matrices? I am really not familiar with standards for network analysis though, and I only think it happened like once or twice that I had to do a actual retake to go around to look what kind of matrix I was dealing with.
(I have no strong opinions, but a general thought I had)

Maybe we could go through and use incidencemat, complexstoichmat, netstoichmat, etc. for these variable names, or maybe standardize the letters we use for each of the matrices and have that somewhere in the documentation (I believe the most common I have seen are Y or Z for the complex stoichiometry matrix, D for the incidence matrix, and S or \Gamma for the reaction stoichiometry matrix)

Except you don't want a variable name to be the same as function names we provide so they should be something different.

Perhaps we should have made the functions get_netstoichmat and such but it is too late now for that as it would be breaking. (And our naming is not inconsistent with how base Julia names many functions.)

TorkelE · 2024-06-03T21:55:50Z

src/network_analysis.jl

+        # Creates, for this conservation law, the sum of all independent species (weighted by
+        # the ratio between the coefficient of the species and the species which is elimianted.
        coefs = @view N[i, indepidxs]
-        terms = sum(p -> p[1] / scaleby * p[2], zip(coefs, indepspecs))


This was probably the function i had to look the most at to understand what was going on. In the end I thought that

terms = sum(coef / scaleby * sp for (coef, sp) in zip(coefs, indepspecs))

(p[1] to coef and p[2] to sp)
was clearer enough that I figured I could propose the change.

TorkelE · 2024-06-04T15:29:46Z

src/network_analysis.jl

 function conservationlaws(nsm::T; col_order = nothing) where {T <: AbstractMatrix}

    # compute the left nullspace over the integers
+    # The `nullspace` function updates the `col_order`.


Is this true @isaacsas? I tried tracking this variable throughout, and looked into the nullspace function, int this is my impression of what happens.
(I was unsure since they call it nullspace and not nullspace!)

TorkelE · 2024-06-04T15:30:11Z

src/reactionsystem.jl

+    j in the i'th conservation law.
+    """
    conservationmat::Matrix{I} = Matrix{I}(undef, 0, 0)
    col_order::Vector{Int} = Int[]


col_order was the only one which I felt unsure of, could you confirm what it is @isaacsas

…_file

TorkelE · 2024-07-14T02:32:46Z

src/reactionsystem.jl

+    j in the i'th conservation law.
+    """
    conservationmat::Matrix{I} = Matrix{I}(undef, 0, 0)
    cyclemat::Matrix{I} = Matrix{I}(undef, 0, 0)


cyclemat I am unsure about

TorkelE · 2024-07-14T02:33:32Z

src/reactionsystem.jl

+    reaction complexes in that connected component.    
+    """
    linkageclasses::Vector{Vector{Int}} = Vector{Vector{Int}}(undef, 0)
    stronglinkageclasses::Vector{Vector{Int}} = Vector{Vector{Int}}(undef, 0)


@vyudu At some point it would be good to have short description of these two, and how they are different from normal linkage classes

TorkelE · 2024-07-14T03:50:32Z

src/network_analysis.jl

-    for rx in rxs
+    # Finds the reactions that are part of teh sub-reaction network.
+    rxinds = sort!(collect(Set(
+        rxidx for rcidx in linkageclass for rxidx in complextorxsmap[rcidx])))


Old:

rxinds = sort!(collect(Set(rxidx for rcidx in linkageclass for rxidx in complextorxsmap[rcidx])))

and new

rxinds = sort!(collect(Set( rxidx for rcidx in linkageclass for rxidx in complextorxsmap[rcidx])))

are identical, but I moved the new line a bit to make the formater less bad (i.e. starting for without indent)

TorkelE · 2024-07-14T03:51:12Z

src/network_analysis.jl

+        rxidx for rcidx in linkageclass for rxidx in complextorxsmap[rcidx])))
+    newrxs = allrxs[rxinds]
+    specset = Set(s for rx in newrxs for s in rx.substrates if !isconstant(s))
+    for rx in newrxs


rxs to newrxs. No other change to this function

TorkelE · 2024-07-14T03:52:56Z

src/network_analysis.jl

-              *
-              "one was not expected.")
+        (scaleby != 0) ||
+            error("Error, found a zero in the conservation law matrix where one was not expected.")


Chaneg to a single error message on a single new line (rather than having it spread out over 3). Again to avoid wonky formating from the formater

TorkelE · 2024-07-14T03:55:43Z

@vyudu If you want some practise in reviewing PRs, this could be a good try. Basically, I have gone through the old network analysis functions, and added additional comments in a few places (as I understand what the code is doing). You probably know this as well as me, so if you had a look you might spot if any of my comments are inaccurate.

Don't feel that you have to/should do though. But if you thought it would be fun to try and review a PR (if only a little of it), this could be one to try.

isaacsas

This is adding a ton of comments lengthening the code that violate a major rule of commenting: “Comments should not duplicate the code.”

Please make sure your comments are about explaining what a complicated block of code is doing, or why a given choice was made in code, not simply restating in words what relatively simple code already shows.

isaacsas · 2024-07-14T08:24:41Z

Perhaps look at some articles about commenting for guidelines on their use: https://stackoverflow.blog/2021/12/23/best-practices-for-writing-code-comments/

TorkelE · 2024-07-14T12:50:55Z

Thanks for the link, I read through it. It is good and I mostly agree.

I don't think this is a problem with the PR though? I read through the file from start to finish, and if there was a bit which was not directly clear to me what it did from the code, I added comments to it. There areen't cases of

    # returns the complex stoichiometry matrix.
    nps.complexstoichmat
end

or similar.

If there are places where you think a comment is superfluous, please tell me. But I really have tried to keep a balance between adding comments adding superfluous stuff, and the file is not significantly longer thorough this PR.

isaacsas · 2024-07-14T12:53:28Z

Ok. I think there are a lot of superfluous comments here, so will leave a detailed review when I have time.

TorkelE · 2024-07-14T12:55:09Z

If there is any that you see directly you can point it out and I can try to adapt with that info, or explain. But yes, otherwise just go through when you have time, there is no hurry.

init

5ef4f1a

TorkelE commented Jun 3, 2024

View reviewed changes

up

9ba924a

TorkelE commented Jun 3, 2024

View reviewed changes

TorkelE added 3 commits June 4, 2024 10:57

save progress

245b12d

up

4164400

spelling fixes

8478423

TorkelE commented Jun 4, 2024

View reviewed changes

TorkelE changed the title ~~Add comments to network analysis file~~ [Ready] Add comments to network analysis file Jun 5, 2024

TorkelE changed the title ~~[Ready] Add comments to network analysis file~~ [Non-urgent, Ready] Add comments to network analysis file Jun 11, 2024

TorkelE added src comment additions labels Jul 6, 2024

TorkelE changed the title ~~[Non-urgent, Ready] Add comments to network analysis file~~ Add comments to network analysis file Jul 6, 2024

TorkelE added 3 commits July 13, 2024 08:33

Merge branch 'master' into src___comment_additions___network_analysis…

d0400a2

…_file

up

4603532

up

61fb536

TorkelE commented Jul 14, 2024

View reviewed changes

TorkelE force-pushed the networkanalysis_file_update branch from 84d0eb3 to 61fb536 Compare July 14, 2024 03:35

merge fixes

a4120b9

TorkelE commented Jul 14, 2024

View reviewed changes

another merge thinf

a0ab5da

isaacsas requested changes Jul 14, 2024

View reviewed changes

		@@ -122,6 +136,7 @@ function reactioncomplexes(::Type{SparseMatrixCSC{Int, Int}}, rn::ReactionSystem
		complexes, B

Uh oh!

Conversation

TorkelE commented Jun 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TorkelE commented Jul 14, 2024

Uh oh!

isaacsas left a comment

Choose a reason for hiding this comment

Uh oh!

isaacsas commented Jul 14, 2024

Uh oh!

TorkelE commented Jul 14, 2024

Uh oh!

isaacsas commented Jul 14, 2024

Uh oh!

TorkelE commented Jul 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants