search

modsim / FluxML

A Universal Modeling Language for Metabolic Flux Analysis
MIT License
9 stars 6 forks source link

No time stamp is added for parameter measurements #13

Open ANTS-ON opened 4 years ago

ANTS-ON commented 4 years ago

For flux/pool (ratio) measurement groups no time stamp is added (getTimeStampSet returns an empty set). It would be better to add -1 to the set, because you can than generically get measurement data by iterating over the set. Right now, this works for all measurements but the parameter measurements. Also, it does not make sense to even have such a set if it would supposed to be empty in every scenario.

mbeyss commented 4 years ago

Due to the steady state assumption fluxes and pool sizes are constant over time, hence a flux/poolsize (ratio) measurement is valid at any time point.

(side note: if FluxML is ever extended to the metabolically instationary case fluxes and pool sizes will become time dependent)

Returning -1 would imply the measurement is valid only for t = infinity, i.e. the isotopically stationary case. (side note2: I actually do not like that -1 represents infinity, because there is std::numeric_limits::infinity which states that in a much more understandable way)

I would say that these measurements do not need the getTimeStampSet method at all, because it does not apply. It is inherited from MGroup and was actually there long before timestamps where supported at all. So one way to solve it would be to move all timestamp related members and methods to the MetaboliteMGroup (with unknown side effects) If we leave the method, I for one, am not that unhappy with it returning an empty set, because it kind of represents that this method does not really apply. Could you elaborate, why exactly you would like to iterate over the timestamp set to get all measurements? Alternatively, we could set a special value to represent that. Perhaps NaN, because that does not compare to anything? Or we could have a class on its own to represent timestamps, that ensures that values are always >=0 and has a way to represent infinity and a 'does not apply'. In both cases there are some constraints to the set of timestamps. While infinity could be combined with any actual timestamp, a set that contains the 'does not apply' can not contain anything else.

Thoughts? Oppinions?

ANTS-ON commented 4 years ago

Ok, I understand why -1 would be inconsistent. I like your idea of using NaN as a "pseudo timestamp" for measurements of time-independent unknowns. i must admit that the iteration example was not very though out. I had a little idea that I now see would have never worked out. When it comes to the availability of the getTimeStampSet method I think it should be only available when it can be applied. However, due to backward compatibility and the work of refactoring I think it is better to keep it and just add NaN (which would be an equivalent of "does not apply").

mbeyss commented 4 years ago

I'll have a look. At first glance this seems easy to do.