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

fix: correct capitalization metBiGGID #299

Merged
merged 2 commits into from
Jun 9, 2020
Merged

fix: correct capitalization metBiGGID #299

merged 2 commits into from
Jun 9, 2020

Conversation

edkerk
Copy link
Member

@edkerk edkerk commented Jun 8, 2020

Main improvements in this PR:

Fix:

  • Correct capitalization of metBiGGID field for model conversion.

I hereby confirm that I have:

@edkerk edkerk added the bug Bug that needs fixing. label Jun 8, 2020
@edkerk edkerk requested a review from BenjaSanchez June 8, 2020 09:54
Copy link
Contributor

@BenjaSanchez BenjaSanchez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edkerk I was curious as to why this was not a problem in my setup before, and realized that because the name of the file COBRA_structure_fields.csv is the same in RAVEN as in COBRA, ravenCobraWrapper was calling the file in COBRA instead of the one in RAVEN (generating no error). Why have this duplicity? If we don't want to rely on needing COBRA installed for using the wrapper, then I would suggest changing the name of the file, to avoid more mishaps.

@simas232 simas232 added this to the v2.4.1 milestone Jun 8, 2020
@simas232
Copy link
Collaborator

simas232 commented Jun 8, 2020

@BenjaSanchez, @edkerk, we may have a function which checks if COBRA toolbox is available in MATLAB pathlist, e.g. isCOBRAInstalled. It is very likely that we would need this function in more situations than this.

@edkerk
Copy link
Member Author

edkerk commented Jun 8, 2020

@BenjaSanchez Good questions. The file is supposed to be exactly the same as the one from COBRA (I missed that I manually changed metBiGGID when I added the file here, and the reason I didn't notice this is indeed because of your explanation).

The reason to distribute the same file with RAVEN is indeed to prevent reliance on COBRA for ravenCobraWrapper (one might want to get the annotations in metBiGGID format instead of metMiriams). Why do we use the exact same file with the same file name? When RAVEN is starting to function as submodule in COBRA (#184), but there is still a need to move between model formats, then we can just directly rely on the COBRA-distributed file and remove the RAVEN-distributed version.

Nonetheless, I don't like to take files from the MATLAB path if there is a risk that there could be multiple files by the same name. I'm therefore refactoring ravenCobraWrapper to specifically use the RAVEN version.

@simas232 Good suggestion, but I don't think it's required here.

- include path to files for ravenCobraWrapper and standardizeModelFieldOrder
- freshly copy-paste COBRA_structure_fields.csv from COBRA develop branch
@edkerk edkerk merged commit 474b3ba into devel Jun 9, 2020
@edkerk edkerk deleted the fix/COBRAfields branch June 9, 2020 20:56
@simas232 simas232 mentioned this pull request Nov 21, 2020
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug that needs fixing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants