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

Allow algorithmParameter.value to be an empty string (meaning null) #88

Open
jonrkarr opened this issue Dec 23, 2020 · 13 comments
Open
Labels

Comments

@jonrkarr
Copy link
Contributor

In my opinion, it should be possible to express that the value of an algorithm parameter should be null. For example, null could be used to indicate that a random algorithm shouldn't be seeded with a specific number or to indicate that an algorithm should have a preset maximum number of iterations.

An empty string is would be a convenient way to encode this. However, libSED-ML doesn't allow this.

Historically, in such cases, numerical programs have often used 0 to represent null (e.g., 0 is often used to indicate no maximum number of reactions). However, this 0 can be confusing in such contexts because 0 is typically the same data type as all other values, and marginally greater values have qualitatively different meanings. I think null would more clearly express the semantic meaning of such parameter values.

@fbergmann
Copy link
Owner

Indeed, since the actual value is stored as string internally, i have been determining whether the attribute is set or not, by whether the element is empty or not. so the function isSetValue would return false, for an empty string. This can be changed, I'm just not sure that this would be the right thing. (Incidentally isSetValue() returning false, is exactly what would be needed in this use case so that the program does the right thing).

rather than using an empty parameter value to denote that a default should be used, i would just not specify it. That way (since the tools can't rely on a specific algorithm parameter being there) the tool will take its default value (so you would get exactly the behavior you want by not specifying the parameter). Otherwise I could imagine naive implementations to just convert the empty string to a number, which (depending on programming language) might just result in 0.

Maybe this needs a wider discussion.

@jonrkarr
Copy link
Contributor Author

Explicitly declaring that a parameter value should be null is different than not specifying a value so that the default is used.

Reading the SED-ML specification PDF, I see that values are intended to be string representations of numbers. I missed this because libSED-ML treats value as a string. There are many algorithms for which this is insufficient. For example, a value could be a KISAO id to indicate a sub-algorithm that should be used. Some algorithms even have parameters that are not scalars such as the reaction partitioning parameter of the COPASI. Supporting other data types is essential for broader use of SED-ML.

@fbergmann
Copy link
Owner

Indeed, and in L1V4, we allow for string parameters, and recursive definition of algorithm parameters.

https://github.com/SED-ML/sed-ml/raw/master/specification/level-1-version-4/sed-ml-L1V4.pdf

still it makes no mention of the case of the empty string (or NULL). Maybe it should (as at least i'm unclear of what to do in that case).

@luciansmith
Copy link
Collaborator

I don't think an empty string should mean 'null' for algorithm parameters, If the parameter is a Boolean, the strings 'true' or 'false' should be used. If there's some other parameter that really needs to be null, maybe use the string "NULL" or "none" or something? What's the example parameter that needs this?

@jonrkarr
Copy link
Contributor Author

jonrkarr commented Apr 2, 2021

The string null would be fine.

Null is the default value of a variety of parameters. Even though it is the default value, I think it should be possible for a SED-ML document to explicitly say that the value of a parameter should be null. I think this is something that non-SED-ML experts would likely expect since a variety of tools support something similar.

Examples:

  • Stop condition: null indicates no additional stop condition beyond the maximum time
  • factor of parsimonious FBA
  • initial, min, max step size: some implementations use null to indicate that the algorithm should choose; others use 0 to indicate this (this may be convention, but this convention is likely unclear to non-experts)
  • random number generator seed: null means don't see generator

@matthiaskoenig
Copy link

I don't like the idea at all.
Could we not just add a Kisao Term AlgorithmParameterUndefined or Undefined which we could set in such cases. Then we don't have to deal with all the None, empty strings, what does it mean for SetValue and so on interpretation.
The cleanest solution is to be explicit and if you want to say that and AlgorithmParameter is undefined just use the corresponding KISAO term. This will make interpretation and implementation much cleaner instead of relying on side effects.

@jonrkarr
Copy link
Contributor Author

jonrkarr commented Apr 3, 2021

I agree with being explicit. My preference would be to more explicitly capture data types.

To be consistent with KiSAO and many other ontologies, such a term should have an id like KISAO_000XXXX. Granted SED-ML documents are not intended to be read by humans, but the semantic meaning of the usage of such a KiSAO term would be less obvious to a reader than undefined, null, none, xsi:nil="true" or similar.

@luciansmith
Copy link
Collaborator

The idea of having a KiSAO term that means 'none' is a whole new use of KiSAO: up until now, all KiSAO terms have been parameters that need values, not a value that is applied to a parameter. You'd end up with something like:

<algorithmParameter kisaoID="KISAO:0000211" value="KISAO:9999999"/>

It's certainly still possible, but seems a little odd to me.

At this point, I've lost the plot of what the suggestion is and who approves of which suggestion. It seems clear that the title of the issue is no longer a request, though, which means that we should move the discussion to a new tracker item for SED-ML itself, instead of Frank's libsedml library. I'd make one myself, but I'm not sure which option people like best at this point.

@jonrkarr
Copy link
Contributor Author

jonrkarr commented Apr 6, 2021

I started the issue here because the SED-ML specifications don't say that value must be a non-empty string. To me, this seemed like a simple issue where libSED-ML diverges from what's stated in the SED-ML specifications. Alternatively, if value is intended to be non-empty, then I think the SED-ML specifications need to say this.

I agree creating a KiSAO term to mean null is strange. This would be difficult to read and not consistent with any existing KiSAO term. I would not be in favor of using KiSAO this way.

@fbergmann
Copy link
Owner

I'm happy to implement whatever is decided in the clarification of the specification.

@matthiaskoenig
Copy link

Thanks for the clarifications. Yes a KISAO term does not make sense for the value.

For me as a tool developer seeing an emtpy string or null, None string as value of an AlgorithmParameter would just mean to not do anything, but ignore the parameter. So it would be equivalent to not having the AlgorithmParameter and would not provide any additional value.
So I have a bit problem to understand what functionality this would add and what the expected behavior of a tool would be in such a case.

@jonrkarr
Copy link
Contributor Author

<algorithmParameter value="" /> does not need to have the same meaning as there not being an instance of algorithmParameter at all.

I think its important to keep in mind the needs of other communities that want to use SED-ML with other modeling frameworks and simulation algorithms were the parameters often aren't floats and can include null.

In any case, the SED-ML specifications doesn't say that the value of value has to be a non-empty string.

@luciansmith
Copy link
Collaborator

This is a good point--we should add text to the spec to mention that empty strings are equivalent to not being set.

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

No branches or pull requests

4 participants