Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SBML missing logic arc #736

Closed
ugurdogrusoz opened this issue Jul 15, 2024 · 17 comments
Closed

SBML missing logic arc #736

ugurdogrusoz opened this issue Jul 15, 2024 · 17 comments
Assignees
Labels
Critical Something critical isn't working
Milestone

Comments

@ugurdogrusoz
Copy link
Contributor

ugurdogrusoz commented Jul 15, 2024

(thanks @adrienrougny) Logic arcs are completely missed in SBML notation. These are needed to connect Species to Logical operators (see the bottom middle part here; edges from D & E to the logical operators are of type logic arc). PD has a counterpart for logical operators. When we implement these, we should also add the rules for creating edges of this type (Species to Logical Operators).

@ugurdogrusoz ugurdogrusoz added the Critical Something critical isn't working label Jul 15, 2024
@ugurdogrusoz ugurdogrusoz added this to the version 4.0 milestone Jul 15, 2024
@ugurdogrusoz
Copy link
Contributor Author

This is from @SelbiEreshova:
Yes, indeed I did not implement a 'Logical operator arc'. Rather, I added rules so we can use existing edges (consumption, catalysis for 'and' and 'or' operator, inhibition for 'not' operator) to construct what we see in Boolean logic gate section.
Here is an 'and' operator I constructed on Newt 4:
Screen Shot 2024-07-17 at 10 38 42

We are still talking to find out the motivation for this but we'll properly need to disallow the usage of some arc types on logic gates when we properly implement logic arcs.

@umut-er
Copy link
Contributor

umut-er commented Jul 19, 2024

image

This seems to be a similar issue to #735.

From what I understand, we want the logic arc to connect any node to a logical operator. After that, and, or gates should be able make a single connection to a process node with an edge of type consumption or catalysis. The not gate can use the inhibition edge in addition to those. Is this accurate? Is this the behavior we want? What about trigger edges (see picture above)? What about unknown edges (unknown catalysis, unknown inhibition etc...)? What about reduced edges?

@ugurdogrusoz
Copy link
Contributor Author

See my email and list the rules as you understand here so I can verify.

@umut-er
Copy link
Contributor

umut-er commented Jul 19, 2024

Here is my understanding.

I take that modulating arcs are the second arc division in our current sbml palette. The reduced modulating arcs are the third division.

and / or gates can take as many logic arcs as desired as input but can only have 1 modulating (or reduced modulating) arc as output. not gate can only take in one input logic arc but otherwise is the same as the other ones.

Is that accurate @ugurdogrusoz?

PS. Maybe we should name the second division modulating arcs and the third reduced modulating arcs.

@ugurdogrusoz
Copy link
Contributor Author

Yes that sounds correct. I am not sure about naming that's why the separation without naming.

@umut-er
Copy link
Contributor

umut-er commented Jul 22, 2024

This issue is almost done on my part. However, there is one issue:

image

(The arcs are in the order they are in the palette) As you can see, for the majority of arcs, there is a port connection issue. This is probably because these arcs usually don't originate from a process (see the compartment on the right). However, for a minority of arcs, we get the intended behavior, which means that there is a (relatively easy?) fix out there. I think Noor should take a look at this because he was working on #735, which, from the outside, looks very similar. If these issues are unrelated, I can work on a fix.

umut-er added a commit to iVis-at-Bilkent/sbgnviz.js that referenced this issue Jul 22, 2024
umut-er added a commit that referenced this issue Jul 22, 2024
@umut-er umut-er assigned ugurdogrusoz and unassigned umut-er Jul 22, 2024
@umut-er
Copy link
Contributor

umut-er commented Jul 22, 2024

Assigning to @ugurdogrusoz for now. You can check the rules for connecting logical operators and the logic arc. The port issue I outlined still remains however.

@ugurdogrusoz
Copy link
Contributor Author

ugurdogrusoz commented Jul 22, 2024

@umut-er Looks good in general except:

  • 1. Logical operator --Inhibition--> Omitted process gives an error in the Console and does not work

@ugurdogrusoz
Copy link
Contributor Author

ugurdogrusoz commented Jul 22, 2024

@NoorMuhammad1 Let's make sure

  • 2. All Logical Arcs that connect a Species (Phenotype excluded) to a Logical Operator use a proper port (input port of the Logical Operator). Currently Unknown Logical Operator is the only problem one here.

  • 3. All modulation arcs listed below connect Logical Operators to their target Process nodes (Association and Dissociation excluded) from a port (output port of the Logical Operator). Currently, Unknown Catalysis and Unknown Inhibition are the only problem ones. Also (see comment below for @umut-er about constructing an Inhibition; you should test the situation with this modulation arc after that problem is fixed).

Screenshot 2024-07-22 at 16 31 47

umut-er added a commit to iVis-at-Bilkent/sbgnviz.js that referenced this issue Jul 23, 2024
@umut-er
Copy link
Contributor

umut-er commented Jul 23, 2024

All modulation arcs listed below connect Logical Operators to their target Process nodes (Association and Dissociation excluded) from a port (output port of the Logical Operator). Currently, Unknown Catalysis and Unknown Inhibition are the only problem ones. Also (see comment below for @umut-er about constructing an Inhibition; you should test the situation with this modulation arc after that problem is fixed).

Shouldn't the reduced modulation arcs (i.e. the third group of edges, see my message) also connect to the output port of the logical operator? Also, isn't there a problem with the trigger edge in the row you indicated?

Logical operator --Inhibition--> Omitted process gives an error in the Console and does not work

Should be fixed. It was caused by a typo.

NoorMuhammad1 added a commit to iVis-at-Bilkent/chise.js that referenced this issue Jul 23, 2024
NoorMuhammad1 added a commit to iVis-at-Bilkent/sbgnviz.js that referenced this issue Jul 23, 2024
@NoorMuhammad1
Copy link
Contributor

I fixed both the issues. The problem was that the code looks for different types of pre-defined set of arcs to know if they should be connected at the ports or not. I just added the arcs that were missing from the list and added a new option to check for logic arc.

@ugurdogrusoz
Copy link
Contributor Author

All modulation arcs listed below connect Logical Operators to their target Process nodes (Association and Dissociation excluded) from a port (output port of the Logical Operator). Currently, Unknown Catalysis and Unknown Inhibition are the only problem ones. Also (see comment below for @umut-er about constructing an Inhibition; you should test the situation with this modulation arc after that problem is fixed).

Shouldn't the reduced modulation arcs (i.e. the third group of edges, see my message) also connect to the output port of the logical operator? Also, isn't there a problem with the trigger edge in the row you indicated?

I think not since they are directly between Species, do not go through process nodes.

@ugurdogrusoz
Copy link
Contributor Author

  1. Modulation arcs (including Unknown Catalysis and Unknown Inhibition) now connect Logical Operators from their output ports but they should not use input ports to connect to processes. This can be seen in the related table.
Screenshot 2024-07-23 at 10 45 10

@ugurdogrusoz ugurdogrusoz removed their assignment Jul 23, 2024
NoorMuhammad1 added a commit to iVis-at-Bilkent/chise.js that referenced this issue Jul 31, 2024
@NoorMuhammad1
Copy link
Contributor

The issue for 3. Modulation arcs connecting to ports of processes while joining logical operators with the process is solved. I added the option to check if the arc was a modulation arc and the target node was a process node. And then if it is true then the arc is not connected to the target node ports.

NoorMuhammad1 added a commit to iVis-at-Bilkent/sbgnviz.js that referenced this issue Jul 31, 2024
@ugurdogrusoz
Copy link
Contributor Author

@hasanbalci @umut-er Can you guys test this as well? Looked good to me.

@hasanbalci
Copy link
Contributor

I didn't see any problem either.

@hasanbalci hasanbalci removed their assignment Jul 31, 2024
@umut-er
Copy link
Contributor

umut-er commented Aug 1, 2024

I think it is good to go.

@umut-er umut-er assigned ugurdogrusoz and unassigned umut-er Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Critical Something critical isn't working
Projects
None yet
Development

No branches or pull requests

4 participants