dondi / GRNsight

Web app and service for modeling and visualizing gene regulatory networks.
http://dondi.github.io/GRNsight
BSD 3-Clause "New" or "Revised" License
17 stars 8 forks source link

GraphML and SIF imports run afoul of recent species feature #886

Closed dondi closed 3 years ago

dondi commented 4 years ago

The species feature that was worked on last year does not carry over to our GraphML and SIF import code. The underlying principle of our files is that the only true requirement is a network—everything else should be “icing on the cake” and so should not produce errors if they are missing.

For the GraphML and SIF importers, the meta property which contains the species will be missing because there is flat-out no (standard) way to introduce that value in the files. Thus, the species code that was added last year should gracefully skip itself if the meta property is missing.

For the default behavior, there is existing code in the workbook importer and we should investigate how to refactor GRNsight so that the existing default logic applies across all file formats.

igreen1 commented 3 years ago

This issue happens for accessing .meta (species and taxon) values as well as accessing .expression

In graphml, @igreen1 added empty definitions of .meta and .expression - but this doesn't feel like the best practice. However, now when accessing .meta.species, JS returns undefined instead of error. This undefined is properly handled by the code already built in for undefined species

In sif, @igreen1 struggled to understand the parser so instead, the catch-all solution in update-graph was used. In this case, before attempting to access the variables, there is a check as to whether those variables exist. While this works, there is worry that it is an overly simple path.

I would love some advice on these solutions.

@igreen1 was also was having issues with warnings. The scope of warnings is confusing as there is a warnings.js, but it contains only some of the warnings the system generates

Good news, though, is that it is now working as of #f209f9c

dondi commented 3 years ago

@igreen1 has implemented a fix but needs to update the test suite since this is new behavior.

igreen1 commented 3 years ago

When attempting the build, the test throws the following error

image (1)

This is because the best fix I found, following the notation of the .xslx import, requires an addition of .meta and .expression to the sif and graphml imports. However, the test is expecting a deep equality. The next goal, then, is to add this new format to the tests.

Discussed in the meeting today how to implement this fix; should be a simple paste into the expected results of two empty fields.

igreen1 commented 3 years ago

Officially working on igreen1 commit #6abe1dd

Fixed the test issue by adding expression and meta objects.

Also converted expression and meta imports to objects instead of arrays to more closely reflect the type of data they store.

PR is waiting for some final bug fixes on the graphML validation.

dondi commented 3 years ago

Clarification for @ahmad00m—SIF and GraphML used to report errors when they were loaded because they don't contain species information. This should no longer be an issue.

ahmad00m commented 3 years ago

Tested all test files in GRNsight/ test-files/ import-samples. The graph was drawn correctly and no error was shown for neither the SIF or GraphML formats. However, not quite sure if correct files were tested for this issue. Also, no warning was reported for these formats, but for xlsx format the graph was drawing and a warning was reported for missing species name.

dondi commented 3 years ago

@dondi confirmed that the correct files were tested and it was also confirmed that the missing species error does not apply to SIF and GraphML.