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

Missing Tests for Models with Variables with SId INF or NaN #72

Closed
FFroehlich opened this issue Oct 23, 2020 · 7 comments
Closed

Missing Tests for Models with Variables with SId INF or NaN #72

FFroehlich opened this issue Oct 23, 2020 · 7 comments

Comments

@FFroehlich
Copy link

As far as I can tell the SBML spec allows variables to have the literals INF and NaN as SId, so one could assign those to values different from their MathML definition.
Probably something thats pretty stupid to do and we accordingly do not plan to support such models in AMICI, but this should probably be tested against.

I also don't see how this could even be parsed from XML, so I don't know how any other toolbox would support this. If this is indeed the case it might be reasonable to prohibit this in the SBML spec, since although the models itself would be well defined, it would be technically impossible to implement it correctly.

@fbergmann
Copy link
Member

@FFroehlich if a variable in SBML is called 'INF' or 'NaN' (which is a moronic idea and of no use at all), then that is just on identifier for the variable. And it is not to be confused with the MathML construct. In mathML it can be easily differentiated by the fact, that there will be a NaN , so an identifier called NaN rather than '' ...

so i think the test is still valid, and since it falls into the allowable subset of strings that represent a valid SId, i think it is ok to keep.

@FFroehlich
Copy link
Author

FFroehlich commented Oct 26, 2020

@FFroehlich if a variable in SBML is called 'INF' or 'NaN' (which is a moronic idea and of no use at all),

I agree !

then that is just on identifier for the variable. And it is not to be confused with the MathML construct. In mathML it can be easily differentiated by the fact, that there will be a NaN , so an identifier called NaN rather than '' ...

But that information is lost after processing with libsbml.formulaToL3String, correct? http://sbml.org/Software/libSBML/5.18.0/docs/python-api/namespacelibsbml.html#a5aee8f98b5db063cadbae96c5e000cb3

so i think the test is still valid, and since it falls into the allowable subset of strings that represent a valid SId, i think it is ok to keep.

I am arguing that there actually should be a test! I just wasn't sure whether it is technically possible to pass it, but it sounds like it is.

@fbergmann
Copy link
Member

indeed, that information won't be there when converting the ASTNode to infix. So what would need to happen, is that before running the tests, all oddly named Species / Parameters / Compartments / Reactions would have to be renamed:

  • true, false, nan, inf, time, avogadro, pi

prior to running the simulation. I'm quite sure there were tests for variables named time at least.

And i thought there would test for that ... maybe @luciansmith can provide some insight here. Since all the tests in the 'semantic' branch, are supposed to be simulatable and numerically comparable, maybe there was an issue with comparing inf / nan values?

@FFroehlich
Copy link
Author

indeed, that information won't be there when converting the ASTNode to infix. So what would need to happen, is that before running the tests, all oddly named Species / Parameters / Compartments / Reactions would have to be renamed:

  • true, false, nan, inf, time, avogadro, pi

prior to running the simulation. I'm quite sure there were tests for variables named time at least.

There are tests for time/avogadro but those are csymbols, so they can be correctly parsed by libsbml.formulaToL3StringWithSettings with avocsymbol set to True (this seems to generally affect all csymbols, including time). I don't think there are any tests for pi/true/false/inf/nan

And i thought there would test for that ... maybe @luciansmith can provide some insight here. Since all the tests in the 'semantic' branch, are supposed to be simulatable and numerically comparable, maybe there was an issue with comparing inf / nan values?

@luciansmith
Copy link
Member

Yup, these would be good tests to add. As you say, they would be clear in the XML, and not clear at all in the infix!

luciansmith added a commit that referenced this issue Oct 27, 2020
The last couple crash the SBML Test Runner, due to there being two 'Time' columns, one for time and one for the variable named 'Time'.  I'm not entirely sure if there's a great solution for this, other than to say 'use the first column as time, and all other columns as element ids'.  But it's a thing that the SBML community needs to work around anyway, since you can indeed have a variable named 'Time'.
@luciansmith
Copy link
Member

OK, added these tests to the 'develop' branch: c6b515a

The last two crash the SBML Test Runner, but I'd love to hear back from you if you get these to work on your own system.

@luciansmith
Copy link
Member

These tests are now in the 3.4.0 release.

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

No branches or pull requests

3 participants