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

Refine/fix import/export operations #712

Closed
hasanbalci opened this issue Jun 10, 2024 · 61 comments
Closed

Refine/fix import/export operations #712

hasanbalci opened this issue Jun 10, 2024 · 61 comments
Assignees
Labels
enhancement Enhancement to existing feature
Milestone

Comments

@hasanbalci
Copy link
Contributor

Current export menu is implemented for exporting SBGNML maps to other formats such as SBML, SIF etc. and they should be working well. However, when we added other formats, SIF and SBML, we didn't put any restrictions for users to use export functionality for these types and this currently leads to wrong export operations. For example, user is able to export SBML file to SBGNML but not in the proper way. The output file of this conversion leads to an SBGNML file with map type SBML which is inconvenient. As a solution we need to add support for possible conversions and disable the rest with a warning message.

Minerva converter that we are currently using for some format conversions supports conversion of SBML to SBGN, GPML and CellDesigner. So we can add support for these three conversions (For SBML -> SBGN conversion, we need to check to which SBGNML version Minerva converts). I think, SBML to SIF conversion can be implemented manually.

Other conversions, SIF to all other formats and if there is any from SBML, we need to show a warning to the user that the export is not currently possible.

@hasanbalci hasanbalci added the enhancement Enhancement to existing feature label Jun 10, 2024
@ugurdogrusoz
Copy link
Contributor

ugurdogrusoz commented Jun 13, 2024

To properly understand the current situation, I prepared some tables here and proposed handling of invalid situations. @hasanbalci Please check.

@hasanbalci
Copy link
Contributor Author

One problem I noticed is the following: Before SBML support, when we try to export SBGN maps as SBML, we were using MINERVA service. After the SBML support, we have write functionality for SBML maps and current 'Export as SBML' button uses that functionality. However, when an SBGN map is open in the canvas and if we try to export the map SBML, the app behaves like the current map is SBML and tries to write it with the write functionality provided with SBML support. Instead it should use Minerva converter instead to convert the current SBGN map to SBML.

@ugurdogrusoz ugurdogrusoz changed the title Refine export operations Refine/fix import/export operations Jun 28, 2024
@ugurdogrusoz ugurdogrusoz assigned okg21 and unassigned hasanbalci Jun 28, 2024
@ugurdogrusoz
Copy link
Contributor

ugurdogrusoz commented Jun 28, 2024

After having discussed all varying combinations, here is what needs to change according to this document (same numbering used in the document):

  • 1. Where we see "NA" in export table entries, let's popup a dialog (similar style to existing ones), that says "Not applicable for the current map type!"
  • 2. Use the Minerva service when converting from PD to .sbml and from SBML to .sbgnml (if 0.3 is chosen, make sure to use 0.3 instead of 0.2 in the file).
  • 2b. Minerva service now requires us to use namespaces ("nwt:") for certain things in our .sbgnml files. When writing out we should include these namespaces. When reading back in, however, this should be optional (we should accept both with and without this namespace).
  • 3. Use our native code to convert the native SBML Model to .sbml. Currently misses layout information (should only perform layout when such info is nonexistent).
  • 4. Use our native code to convert .sbml contents to our native SBML model. Currently misses layout information (should only perform layout when such info is nonexistent).
  • 5. Use our local web service to convert PD to CellDesigner. Currently sometimes says Conversion service not available.
  • 6. If the currently active map is not SIF, it does nothing now; instead, we should warn the user with a popup that says "Can only be used on SIF maps!"

okg21 added a commit that referenced this issue Jul 1, 2024
okg21 added a commit that referenced this issue Jul 2, 2024
@okg21
Copy link
Contributor

okg21 commented Jul 3, 2024

For item 2, I encountered two types of problem when using minerva service for PD to SBML conversion.

  1. For some PD samples sbgnml to sbml minerva conversion returns an xml with this note inside:
[Complex:glyph15]	Unknown extension: extraInfo
[Complex:glyph11]	Unknown extension: extraInfo
[glyph32_0:state]	Invalid structural state
[Complex:glyph14]	Unknown extension: extraInfo
[Complex:glyph12]	Unknown extension: extraInfo
[Complex:glyph13]	Unknown extension: extraInfo
[Complex:glyph18]	Unknown extension: extraInfo
[Complex:glyph16]	Unknown extension: extraInfo</p>
  1. For some PD samples Minerva returns an Internal Server Error. For instance, when I try to export the PD sample Drosophila Cell Cycle as SBML, I get the following error:
error: "Internal server error."
error-id: "0a5fb699-3739-42cf-a5f9-a5779b367de1"
reason: "[Complex 89360553-bab4-532a-c6c7-624272c4e43b]\tProblem with exporting bioEntity"

I did some further investigation regarding this, and this happens when elements inside the complex are not linked to reactions in the model. Here is an example: the first image throws the above error with Minerva API, but the second one is exported with no issues.
Screenshot 2024-07-03 at 14 49 09
Screenshot 2024-07-03 at 14 38 48

okg21 added a commit that referenced this issue Jul 4, 2024
…t imports. Item 2: Using Minerva when converting SBML to .sbgnml
@okg21
Copy link
Contributor

okg21 commented Jul 4, 2024

Regarding item 3, I pushed some fixes to sbgnviz for the SBML map to .sbml export. iVis-at-Bilkent/sbgnviz.js@2cc6ec6
Below are the pre-fix and post-fix exports for the SBML mapped Pharmacokinetics sample.
Before fix:
Screenshot 2024-07-04 at 17 10 52
After fix:
Screenshot 2024-07-04 at 17 07 46

@okg21
Copy link
Contributor

okg21 commented Jul 5, 2024

As of now, items 1, 3 and 6 should be working properly.

@ugurdogrusoz
Copy link
Contributor

1: is OK now
3: SBML -export-to-> SBML: still missing layout info, SBML -export-to-> CellDesigner: conversion service is not available
6: is OK now

@umut-er
Copy link
Contributor

umut-er commented Jul 10, 2024

Issue Number 5

First of all, the error message Conversion service not available has nothing to do with the deployment being compromised. It is a generic message we throw when the conversion service is unable to convert the file.

The service we are using to make apparently has some limitations. If you start with an sbgnml file, convert it to a cd file and try to convert it back using the same service, it won't work (see this). We get something like the following error message with the same call stack every time:

Exception in thread "main" java.lang.NullPointerException: Cannot invoke "org.sbml._2001.ns.celldesigner.ListOfLayers.getLayer()" because the return value of "org.sbml._2001.ns.celldesigner.ModelAnnotationType$Extension.getListOfLayers()" is null
        at fr.curie.cd2sbgnml.xmlcdwrappers.ModelWrapper.addBasicLists(ModelWrapper.java:105)
        at fr.curie.cd2sbgnml.xmlcdwrappers.ModelWrapper.create(ModelWrapper.java:62)
        at fr.curie.cd2sbgnml.CD2SBGNML.toSbgn(CD2SBGNML.java:53)
        at fr.curie.cd2sbgnml.CD2SBGNML.convert(CD2SBGNML.java:857)
        at fr.curie.cd2sbgnml.Cd2SbgnmlScript.convert(Cd2SbgnmlScript.java:48)

or

Exception in thread "main" java.lang.NullPointerException: Cannot invoke "org.sbml._2001.ns.celldesigner.ModelAnnotationType.getExtension()" because the return value of "org.sbml.sbml.level2.version4.Model.getAnnotation()" is null
        at fr.curie.cd2sbgnml.xmlcdwrappers.ModelWrapper.addBasicLists(ModelWrapper.java:78)
        at fr.curie.cd2sbgnml.xmlcdwrappers.ModelWrapper.create(ModelWrapper.java:62)
        at fr.curie.cd2sbgnml.CD2SBGNML.toSbgn(CD2SBGNML.java:53)
        at fr.curie.cd2sbgnml.CD2SBGNML.convert(CD2SBGNML.java:857)
        at fr.curie.cd2sbgnml.Cd2SbgnmlScript.convert(Cd2SbgnmlScript.java:48)
        at Main.main(Main.java:7)

However, if you use the samples provided by the web service, such as these, you can see that you can successfully import these.

Here are some things we should look into (I will update this):

  1. Load sample Drosophila Cell Cycle. If you try to export this into cd, it will fail. However, if you delete all the empty state variables (that come with the sample), you will be able to convert it. Furthermore, if you add empty state variables yourself after deleting all the initial ones, you will still be able to convert. Also causes problems in CaM-CaMK dependent signalling to nucleus and MAPK Cascade.
  2. Insulin-like growth factor (IGF) signalling cannot be converted because it is invalid. Use validate map to see for yourself. So is PolyQ proteins interface.
  3. I tested the CellDesigner app to see if models generated there works on the converter. It sometimes works and sometimes doesn't. When I created a map from scratch the conversion worked.

Conclusion

As the developers themselves state, this converter cannot be used to convert multiple times, it causes too many errors. If you generate a cd map from scratch (in the cd application), the conversion will most likely be successful. If you have an old cd file, convert it to the new version in the cd app (sometimes happens when you open maps from the integrated database on the cd app), there is no guarantee on the conversion. I don't really know what can be done to fix this. We would need to do a very detailed analysis of the sbml and celldesigner standard and the conversion code. Other than that, I don't think there is anything wrong with our current deployment. We can maybe look at the empty state variable thing I outlined above.

@umut-er umut-er self-assigned this Jul 10, 2024
@umut-er umut-er mentioned this issue Jul 10, 2024
2 tasks
@ugurdogrusoz
Copy link
Contributor

@umut-er based on your findings, I checked 5.

@hasanbalci
Copy link
Contributor Author

hasanbalci commented Jul 17, 2024

Perhaps due to the changes related to item 3, now when we export SBML Model as .sbml, spinner starts but doesn't stop.

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

umut-er commented Jul 22, 2024

Perhaps due to the changes related to item 3, now when we export SBML Model as .sbml, spinner starts but doesn't stop.

Should be fixed in the next regular build.

@hasanbalci
Copy link
Contributor Author

Perhaps due to the changes related to item 3, now when we export SBML Model as .sbml, spinner starts but doesn't stop.

Should be fixed in the next regular build.

@umut-er I think you didn't build the sbgnviz repo before pushing the change.

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

umut-er commented Jul 23, 2024

I have been working on the layout information issue. This is a status update. Apparently there is already an sbml extension for encoding layouts (you can find a very detailed explanation of it here, which we probably should cite as per this). libsbmljs already implements this extension (the documentation is very lacking however). Using that functionality, I have added some layout information (this is by no means done yet):
The map:
image

Old Export:

<?xml version="1.0" encoding="UTF-8"?>
<sbml xmlns="http://www.sbml.org/sbml/level2/version4" level="2" version="4">
  <model id="model1">
    <listOfSpecies>
      <species sboTerm="SBO:0000252" id="nwtN_09271ecb_560d_47af_9933_cb85352049e0"/>
      <species sboTerm="SBO:0000252" id="nwtN_738b4f4f_240f_4f06_ac73_d0623379d89e"/>
    </listOfSpecies>
  </model>
</sbml>

New Export:

<?xml version="1.0" encoding="UTF-8"?>
<sbml xmlns="http://www.sbml.org/sbml/level2/version4" level="2" version="4">
  <model id="model1">
    <annotation>
      <layout:listOfLayouts xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:layout="http://projects.eml.org/bcb/sbml/level2">
        <layout:layout layout:id="layout_1">
          <layout:dimensions layout:width="274.25" layout:height="141.25"/>
          <layout:listOfSpeciesGlyphs>
            <layout:speciesGlyph layout:species="nwtN_68b25e79_9748_4924_97b7_7c04d1fc6c7c">
              <layout:boundingBox>
                <layout:position layout:x="476.375" layout:y="335.375"/>
                <layout:dimensions layout:width="63.25" layout:height="33.25"/>
              </layout:boundingBox>
            </layout:speciesGlyph>
            <layout:speciesGlyph layout:species="nwtN_f1e0f887_a4c0_41e0_b891_dd57f109b574">
              <layout:boundingBox>
                <layout:position layout:x="687.375" layout:y="227.375"/>
                <layout:dimensions layout:width="63.25" layout:height="33.25"/>
              </layout:boundingBox>
            </layout:speciesGlyph>
          </layout:listOfSpeciesGlyphs>
        </layout:layout>
      </layout:listOfLayouts>
    </annotation>
    <listOfSpecies>
      <species sboTerm="SBO:0000252" id="nwtN_68b25e79_9748_4924_97b7_7c04d1fc6c7c"/>
      <species sboTerm="SBO:0000252" id="nwtN_f1e0f887_a4c0_41e0_b891_dd57f109b574"/>
    </listOfSpecies>
  </model>
</sbml>

Once this is done, we can move on to reading this format of layout (libsbmljs also has functionality to help with that). Our current import functionality cannot read this new file at all (not just the layout information).

PS. This is not up on the internal server. It is too early to push and breaks the import functionality.

PS 2. Is there another web service or something similar that can read .sbml files with layout information? It would be good to test the new export since our import is currently unable to handle this.

@hasanbalci
Copy link
Contributor Author

@umut-er For item 3, we should allow user to export a map even if it is invalid. Because a user might be curating a map and want to save it anytime to continue later. We allow exporting invalid maps in other formats as well.

@umut-er
Copy link
Contributor

umut-er commented Aug 6, 2024

@umut-er For item 2, we can continue as you suggested. But if it will cause problem while importing the cell designer files, is it possible to convert CD style to the style you suggested while reading those files?

I realized that the converter we use for CD files converts them to SBGNML, not SBML. So, the changes we make to the SBML format has nothing to do with whether we are able to import CD files at all. Am I misunderstanding this?

Below is a CD file I just created in the CellDesigner app with a simple association reaction (Just remove the .txt. We can import this directly with drag & drop. The map type is PD after import).
association.xml.txt

@umut-er For item 3, we should allow user to export a map even if it is invalid. Because a user might be curating a map and want to save it anytime to continue later. We allow exporting invalid maps in other formats as well.

Just to be clear, I was only talking about when the map includes reaction nodes with no inputs or outputs . I previously thought libsbmljs did not allow this but I checked and it appears I was wrong. I will implement that.

@hasanbalci
Copy link
Contributor Author

@umut-er Yes, CD converter should be for CD to SBGN conversion. Currently, we cannot import any CD files with SBML import, right? We are expecting to use the CD -> SBGN converter. If this is the case, then there is no problem.

For item 3, please implement it, thanks!

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

umut-er commented Aug 7, 2024

I think we should we good to go on item 2 and 3.

@umut-er
Copy link
Contributor

umut-er commented Aug 8, 2024

I think we can tick 3 and 4 from this. There are no issues left there that I know of.

Also, I would appreciate a brief explanation on the issues left with item 2 on that list. I have never worked on it and I am not very familiar with it.

@hasanbalci
Copy link
Contributor Author

In item 2, we want to make sure that we are using Mınerva conversion service during export of PD maps as SBML and during import of SBML maps as PD. For the first operation, we should be using this code and for the second operation this code (you can see the API calls to Minerva service).
@umut-er Please make sure that we are using these codes and mentioned import and export operations work correctly.

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

umut-er commented Aug 9, 2024

@hasanbalci can you check item 2 now? There was a small issue with the SBML -> .sbgn conversion but it should be fixed now.

@hasanbalci
Copy link
Contributor Author

In item 2, we want to make sure that we are using Mınerva conversion service during export of PD maps as SBML and during import of SBML maps as PD. For the first operation, we should be using this code and for the second operation this code (you can see the API calls to Minerva service). @umut-er Please make sure that we are using these codes and mentioned import and export operations work correctly.

@umut-er I now realized that my explanation here for item 2 is not exactly correct, sorry. We want to make sure that the export from PD to SBML and export from SBML to PD are using Minerva converter.

When testing I realized that while exporting an SBML map as SBGN-ML Plain or SBGN-ML Plain 0.3, Save dialog box remains open after we click on Save button. In addition, I don't remember if this is discussed before but can we add a spinner while exporting a PD map as SBML.

@umut-er
Copy link
Contributor

umut-er commented Aug 9, 2024

Did you rebuild the server? I didn't, I forgot to mention that @hasanbalci

@hasanbalci
Copy link
Contributor Author

@umut-er Hmm, ok. I will test it after rebuild. But one last small request related to this issue: Can we make inferNestingOnLoad automatically true during import of SBML files. I think it is more natural to load it by inferring nesting, otherwise the user will have to remember and check the option every time.

@umut-er
Copy link
Contributor

umut-er commented Aug 9, 2024

@umut-er Hmm, ok. I will test it after rebuild. But one last small request related to this issue: Can we make inferNestingOnLoad automatically true during import of SBML files. I think it is more natural to load it by inferring nesting, otherwise the user will have to remember and check the option every time.

Ok, but I have some questions. Infer nesting on load is a variable of a map, not a format. So, if infer nesting is automatically true, 1) there would be no way to disable it for SBML, or, 2) it would be true by default for every format. Is that OK?

@hasanbalci
Copy link
Contributor Author

If there is no way to define complex nodes as parents in SBML format, I think we should always apply it. Its disabled state is for using parent reference if it is already defined in the map, but if we are sure that there will be no such information, there is no need to disable it.

Let's make this only for SBML, not for every format. You can keep the current option before loading the file, make it true temporarily, and then set it back to the current option. We do it while loading query results from PathwayCommons query as well.

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

umut-er commented Aug 10, 2024

I made it so that we infer nesting on load for SBML files always if there is layout information. Naturally, we can't infer nesting if there is no layout information. These changes will be live ~2hrs later from the time of this message.

The issues you mentioned here seems to be resolved @hasanbalci.

@hasanbalci
Copy link
Contributor Author

@umut-er Everything looks good, thanks! Only I cannot see spinner while exporting a PD map as SBML. If we can also add it, I think we can close this issue.

@hasanbalci
Copy link
Contributor Author

hasanbalci commented Aug 10, 2024

I also noticed that if I try to connect two macromolecules with a consumption edge (without a process in between) in a PD map, the error dialog box says "Conversion failed!". It was "Invalid edge type! Refer to Legend menu for help." before. I think there is something wrong there. The same is valid for other formats as well. An invalid attempt to add a graph element shows "Conversion failed!".

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

umut-er commented Aug 10, 2024

Both issues are now fixed on the internal server.

@hasanbalci
Copy link
Contributor Author

hasanbalci commented Aug 10, 2024

@umut-er Thanks, both issues are now ok.
I think the import/export functionality is now in a pretty good state. @ugurdogrusoz If you want you can do a final test of the cases and close the issue.

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

Issues with testing export/import will be separately reported, closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to existing feature
Projects
None yet
Development

No branches or pull requests

4 participants