Implemented working prototype of 3.X sub_component <---> 2.X component conversion #282
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… a test file, and wrote a unit test
…te a file in a temporary directory
jakebeal
reviewed
Sep 22, 2024
| try: | ||
| self.visit_component_reference(feature) | ||
| except NotImplementedError as e: | ||
| # highlights the error message in red. |
There was a problem hiding this comment.
I don't believe that you should be swallowing the error here and turning it into a print statement.
| print(f"\033[91m{e}\033[0m") | ||
| else: | ||
| raise NotImplementedError( | ||
| 'Conversion of Component features from SBOL3 to SBOL2 not yet implemented') |
There was a problem hiding this comment.
Suggested change
| 'Conversion of Component features from SBOL3 to SBOL2 not yet implemented') | |
| f'Conversion of Component feature of type {type(feature)} from SBOL3 to SBOL2 not yet implemented') |
| try: | ||
| self.visit_interaction(interaction) | ||
| except NotImplementedError as e: | ||
| print(f"\033[91m{e}\033[0m") |
There was a problem hiding this comment.
Same as above - do not swallow errors here, or below
| identity_mappings[sub3.identity] = comp2.identity | ||
|
|
||
| def handle_subcomponent_identity_triple_surgery(self, identity_mappings): | ||
| with tempfile.TemporaryDirectory() as tmpdir: |
There was a problem hiding this comment.
I don't think that you should be reading and writing tempfiles here. This could result in large numbers of complete read-write cycles in the course of a single conversion and be extremely slow.
I would suggesting saving up all of the identity surgery plans into a single shared datastructure, then doing the surgery on the graph like it's done over here:
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I implemented a first working version of
sub_component<--->componentconversionA Unit test for a round trip starting from SBOL2 still needs to be written.
In addition, the prototype does not handle
hasLocationandorientationproperties.For the time being, I am conveerting all
sub_componentsinto 2.Xcomponents. However, this is not always the case, since "SBOL 2.x Component, Module, and FunctionalComponent objects map to SBOL 3.x SubComponent objects" as per the documentation.I don't necessarily feel comfortable merging yet, but I would appreciate feedback on what I have at the moment.