PathwayCommons / factoid-converters

Web services for Factoid project to convert between JSON, BioPAX, SBGN data formats
http://biopax.baderlab.org/factoid-converters/
MIT License
2 stars 1 forks source link

In test2.js for Other Interaction type having both participants and controller/target fields is causing redundancy #4

Closed metincansiper closed 5 years ago

metincansiper commented 5 years ago

I realized that in test2.js for Other Interaction type there is both participant field and controller/target fields where participants is the union of controller/target.

Is it just a redundancy in the test file or having both of the fields were expected?

Actually controller and target fields are meaningful only when the controlType is set. Participants are not supposed to be separated marked as controller/target when it is set.

I think there are 2 alternative ways to go. We can either have only the participants field anyways and if the controlType is set we use the first participant as controller and the second one as the target (since the participants field represents an array). Or we expect to have participants field in the input when controlType is not set and expect to have controller and target types when it is not set.

I am closer to the first idea as it is a more consistent way of representation. @IgorRodchenkov what do you think?

IgorRodchenkov commented 5 years ago

You're probably right. I thought this redunnancy is not critical (as this json format is still kinda internal one) We'd remove the controller and target, no problem. But having those may give you flexibility to deal with #3, who knows how it'll go, it does not hurt (just ignore those fields), right? Also, with one or both controller and target , you don't worry about the order of participants in the participants list. @metincansiper

metincansiper commented 5 years ago

I suppose having controller and target fields will not give me an extra flexibility in implementing #3. Therefore, I am using the participants order instead of having these fields if you do not have any preference.

IgorRodchenkov commented 5 years ago

No preference as long as the order is guaranteed and consistent; so let me double check this in factoid model and make sure.

IR

On Nov 14, 2018, at 5:43 PM, metincansiper notifications@github.com wrote:

I suppose having controller and target fields will not give me an extra flexibility in implementing #3. Therefore, I am using the participants order instead of having these fields if you do not have any preference.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or mute the thread.

metincansiper commented 5 years ago

The related code segment of factoid is here (https://github.com/PathwayCommons/factoid/blob/unstable/src/model/element/interaction-type/interaction.js#L13). I do not believe that we can expect the participants to be ordered as [source, target] there.

However, I think that inside of that method must be updated anyways.

If the interaction is not signed we can use the participants list as is, but if it is signed we can use an ordered list of [source, target].

IgorRodchenkov commented 5 years ago

I am working this... Redundancy is not a big deal (anyway, I could try to remove), and you're right reagrding the controlType. Just confirmed with @maxkfranz that the order in the participants in factoid model is not guaranteed, but I can make it consistent at least inside the "other' intn type's toBiopaxTemplate method, using source/target if these are set (but this is still just for the removing small redundancy's sake and probably does not worth the effort...).

IgorRodchenkov commented 5 years ago

@metincansiper please pull/merge the latest changes from master to your fork/branch

IgorRodchenkov commented 5 years ago

So I used the latest 'unstable' factoid (my fork) and manually created a network and exported to json using doc.toBiopaxTemplates(), and saved to the test2.json.

test2_jun-bmp2-etc

BMP2 activates Estradiol . JUN inhibits the transcription/translation of BMP2 . Estradiol activates LEP . BMP2 inhibits Progesterone . Progesterone activates LEP . BMP2 inhibits IGF1 . BMP2 inhibits FSHB . IGF1 activates Progesterone . FSHB activates Progesterone . IGF1 binds with FSHB .