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

No time stamp is added for parameter measurements #13

Open
ANTS-ON opened this issue Jul 23, 2020 · 3 comments
Open

No time stamp is added for parameter measurements #13

ANTS-ON opened this issue Jul 23, 2020 · 3 comments
Assignees

Comments

@ANTS-ON
Copy link
Collaborator

ANTS-ON commented Jul 23, 2020

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
Copy link
Contributor

mbeyss commented Jul 23, 2020

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
Copy link
Collaborator Author

ANTS-ON commented Jul 23, 2020

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 mbeyss self-assigned this Jul 23, 2020
@mbeyss
Copy link
Contributor

mbeyss commented Jul 23, 2020

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

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

2 participants