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

FBC: Can't make package version 3 object in L3v2 core model. #360

Closed
luciansmith opened this issue Jan 18, 2024 · 4 comments · Fixed by #362
Closed

FBC: Can't make package version 3 object in L3v2 core model. #360

luciansmith opened this issue Jan 18, 2024 · 4 comments · Fixed by #362

Comments

@luciansmith
Copy link
Member

It's impossible to create a flux objective and add it to an L3v2 model:

  • If you run createFluxObjective(), it gives you an object with a getLevel() of 3, a getVersion() of 2, and a getPackageVersion() of 3. When you call 'setVariableType', it notices that getVersion is 2, and returns LIBSBML_UNEXPECTED_ATTRIBUTE.
  • If you create a FluxObjective with FluxObjective(3,1,3), it lets you call 'setVariableType', but doesn't let you call addFluxObjective(), because of the version mismatch to the core SBML object.

I propose that packages in general should never check the core level or version; they should only check the package version. (Perhaps the layout/render functions could check the core level, but even they shouldn't check the core version.) Putting 'version1' in the namespace string turns out to be a terrible idea, but that ship has sailed; at this point, all we can do is just not check it.

For the immediate problem, I say we don't check versions when calling 'addXXX', nor when calling 'setVariableType' for FBC objects, but a more thorough scouring of the code will probably be necessary to check for other packages moving forward.

@skeating @fbergmann Opinions?

@fbergmann
Copy link
Member

I agree, checking the packageVersion is definitely sufficient.

@luciansmith
Copy link
Member Author

Can we do this for all packages, not just FBC?

@luciansmith luciansmith reopened this Aug 5, 2024
@fbergmann
Copy link
Member

I had no PR for anything else, I was going to just include the PRs we had open and make a release since it was requested by some. That include the changes we had. If you'd rather add things here. Then go ahead. As long as the package URIs remain untouched I'm fine with you adding other packages to this. If you won't get to it by this Thursday though, I would put it as issue in the issue tracker for another time. As otherwise I won't be able to make the release.

@luciansmith
Copy link
Member Author

Done! #380

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

Successfully merging a pull request may close this issue.

2 participants