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: inter-OS issues #96

Closed
4 tasks done
BenjaSanchez opened this issue Apr 21, 2018 · 8 comments · Fixed by #122
Closed
4 tasks done

fix: inter-OS issues #96

BenjaSanchez opened this issue Apr 21, 2018 · 8 comments · Fixed by #122
Assignees
Labels
fixed in devel this issue is already fixed in devel and will be closed after the next release format fix things associated to format of any of the model/data/script files question ideas/feedback from other people would be appreciated

Comments

@BenjaSanchez
Copy link
Contributor

Description of the issue:

When switching between MAC and Windows machines to save the model, stoichiometric coefficients that use scientific notation are stored differently in the .xml file: MAC stores them as, e.g., 6e-05, but Windows stores them as 6e-005. This creates unnecesary changes in the .xml file when diffing (example here), eventhough the .txt file (and now also the .yml file) remain the same. We could adapt saveYeastModel() to fix this after calling writeCbModel(), but maybe there is a better way... @tpfau do you know if this is due to COBRA toolbox or libSBML? As the .txt file remains the same I would think it's due to the latter. Would there be in either case an easy fix?

Example:

Trying an IO cycle in Windows and MAC:

model = loadYeastModel;
saveYeastModel(model);

I hereby confirm that I have:

  • Tested my code with all requirements for running the model
  • Done this analysis in the master branch of the repository
  • Checked that a similar issue does not exist already
  • If needed, asked first in the Gitter chat room about the issue
@BenjaSanchez BenjaSanchez added format fix things associated to format of any of the model/data/script files question ideas/feedback from other people would be appreciated labels Apr 21, 2018
@BenjaSanchez BenjaSanchez self-assigned this Apr 21, 2018
@tpfau
Copy link
Contributor

tpfau commented Apr 23, 2018

@BenjaSanchez
If its stoichiometric coefficients, than it is likely not in COBRA, as those are passed (as is) into OutputSBML.
However, this is also not necessarily due to libsbml, if this is some OS convention on number representation and libsbml uses a fixed output routine that gives different results on mac vs Windows.
Maybe @skeating knows something here, as I have never noticed this behaviour.

@BenjaSanchez BenjaSanchez mentioned this issue May 30, 2018
3 tasks
@BenjaSanchez
Copy link
Contributor Author

update: PR #117 modifies saveYeastModel.m to process the .xml file after writeCbModel.m. Definitively not optimal, but the only solution available on our side. This issue will be closed on the next release.

@tpfau
Copy link
Contributor

tpfau commented May 30, 2018

@BenjaSanchez
Do you by chance have a mac and a non-mac mat file of the model that goes into writeCbModel?
Just to see if there might be something off before it goes into the saving routine.

@BenjaSanchez
Copy link
Contributor Author

@tpfau I only have windows-generated .mat files, but in GECKO we also had to address this issue, and there we are always saving from the same .mat structure both in windows and MAC, which makes me think that this is an issue with writing, not reading. Furthermore, below a screenshot on the matrix format with a windows machine, which is the same as the MAC format, however it is when saving that this changes in windows:
image

If you want to do further analysis, the .mat in master is generated with windows, and if you load the .xml with MAC you get the other one. If you don't have a MAC machine maybe @hongzhonglu can assist?

@tpfau
Copy link
Contributor

tpfau commented May 30, 2018

I can reproduce this issue on win (00X) vs linux (0X)
Personally, I would not fix this manually but wait for a fix from the SBML crew. The fix you currently have could well kill some annotations that have a format -00whatever. So I would suggest to leave this open until a libsbml patch is available.
I'll contact @skeating about it, maybe she can identify where this is going wrong.

@BenjaSanchez
Copy link
Contributor Author

BenjaSanchez commented May 30, 2018

@tpfau thanks for spotting that exception. I have updated saveYeastModel.m (commit ecf9ed5) so now it only changes lines that contain [0-9]e-00[0-9] once, with that we should not have any problems matching other things. I would rather merge this still, and as soon as SBML fixes it we can roll back. This because we are doing a lot of development between MAC and windows, and to not fix it with this patch impairs for us properly diffing between branches.

@tpfau
Copy link
Contributor

tpfau commented May 31, 2018

I would suggest to use regexprep instead of strrep and test for the - sign instead of only changing those with one. I know, it is highly unlikely, that you will have large numbers in there, but it could happen.

        if length(regexp(inline,'[0-9]e-?00[0-9]')) == 1 && length(regexp(inline,'e-?00')) == 1
            inline = regexprep(inline,'(?<=[0-9]e-?)00(?=[0-9])','0');
        end

You could even check for the stoichiometry tag, to ensure, that this really only happens with stoichiometries (and thereby skipping the first regexp checks):

            inline = regexprep(inline,'(?<=stoichiometry="[0-9]+(\.[0-9]+)?e-?)00(?=[0-9])','0');

@BenjaSanchez
Copy link
Contributor Author

@tpfau thanks for the nice improvement! I have included your first option (minus the second check as it became unnecessary): It might happen in the future that also lower bounds or other properties are introduced in scientific format, so good to prevent those as well. Hopefully we can revert all of this once SBML looks more into it :)

@BenjaSanchez BenjaSanchez added the fixed in devel this issue is already fixed in devel and will be closed after the next release label Jun 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed in devel this issue is already fixed in devel and will be closed after the next release format fix things associated to format of any of the model/data/script files question ideas/feedback from other people would be appreciated
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants