{"action":"created","issue":{"url":"https://api.github.com/repos/kroneckerbio/kroneckerbio/issues/47","labels_url":"https://api.github.com/repos/kroneckerbio/kroneckerbio/issues/47/labels{/name}","comments_url":"https://api.github.com/repos/kroneckerbio/kroneckerbio/issues/47/comments","events_url":"https://api.github.com/repos/kroneckerbio/kroneckerbio/issues/47/events","html_url":"https://github.com/kroneckerbio/kroneckerbio/issues/47","id":103149816,"number":47,"title":"Revert to commit 1664250 and properly pull in changes","user":{"login":"xpspectre","id":4530441,"avatar_url":"https://avatars.githubusercontent.com/u/4530441?v=3","gravatar_id":"","url":"https://api.github.com/users/xpspectre","html_url":"https://github.com/xpspectre","followers_url":"https://api.github.com/users/xpspectre/followers","following_url":"https://api.github.com/users/xpspectre/following{/other_user}","gists_url":"https://api.github.com/users/xpspectre/gists{/gist_id}","starred_url":"https://api.github.com/users/xpspectre/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/xpspectre/subscriptions","organizations_url":"https://api.github.com/users/xpspectre/orgs","repos_url":"https://api.github.com/users/xpspectre/repos","events_url":"https://api.github.com/users/xpspectre/events{/privacy}","received_events_url":"https://api.github.com/users/xpspectre/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/kroneckerbio/kroneckerbio/labels/change","name":"change","color":"5319e7"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":4,"created_at":"2015-08-26T01:31:26Z","updated_at":"2015-08-26T23:39:37Z","closed_at":null,"body":"No, the recent changes to analytic models are not OK.\r\n\r\nTopics in issue #44 do not represent any consensus, especially the requirement that species names be globally unique. Changes, esp. requiring unique names and use of IDs in expressions make the users' job harder at the expense of the developers'. Using kroneckerbio is more important than developing it, so I see these changes as unacceptable. Parsing code can be improved, and the user can always fall back to using IDs if they want. If we want to go the route of formal grammar, we can (probably with a Java dependency, and we can also consider dropping libSBML for directly parsing the parts of SBML we want to handle).\r\n\r\nThe symbolic intermediate exists primarily to encapsulate logic needed for outside formats to interface with kroneckerbio's analytic and massaction models. It serves as a shim between the concerns of model usage (inside kroneckerbio) and model construction/conversion/loading (outside kroneckerbio). A more object oriented implementation of models and converters would probably help with this kind of organization."},"comment":{"url":"https://api.github.com/repos/kroneckerbio/kroneckerbio/issues/comments/135206066","html_url":"https://github.com/kroneckerbio/kroneckerbio/issues/47#issuecomment-135206066","issue_url":"https://api.github.com/repos/kroneckerbio/kroneckerbio/issues/47","id":135206066,"user":{"login":"xpspectre","id":4530441,"avatar_url":"https://avatars.githubusercontent.com/u/4530441?v=3","gravatar_id":"","url":"https://api.github.com/users/xpspectre","html_url":"https://github.com/xpspectre","followers_url":"https://api.github.com/users/xpspectre/followers","following_url":"https://api.github.com/users/xpspectre/following{/other_user}","gists_url":"https://api.github.com/users/xpspectre/gists{/gist_id}","starred_url":"https://api.github.com/users/xpspectre/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/xpspectre/subscriptions","organizations_url":"https://api.github.com/users/xpspectre/orgs","repos_url":"https://api.github.com/users/xpspectre/repos","events_url":"https://api.github.com/users/xpspectre/events{/privacy}","received_events_url":"https://api.github.com/users/xpspectre/received_events","type":"User","site_admin":false},"created_at":"2015-08-26T23:39:37Z","updated_at":"2015-08-26T23:39:37Z","body":"##### Unique species names\r\nThis:\r\n```\r\nm = AddState(m, 'glucose', 'ecm', 1);\r\n```\r\nis better than this:\r\n```\r\nm = AddState(m, 'ecm_glucose', 'ecm', 1);\r\n```\r\n\r\n##### Species names and reactions\r\n> I have yet to meet someone quickly who gets how compartments work with respect reactions.\r\n\r\nYou met him. In fact, as far as I can tell, no one I've ever met has had a hard time figuring out compartments for reactions when building models. It can be made better; refer to my comments on default compartments. In summary, these function signatures are better (cell.glucose also exists, cell is the \"default\" compartment, double-quotes aren't required if the name appears by itself instead of in an expression):\r\n```\r\nm = AddReaction(m, {'ecm.glucose', 'receptor'}, 'glucose:receptor', 'k1', 'membrane');\r\nm = AddReaction(m, {'ecm.glucose', 'membrane.receptor'}, 'membrane.glucose:receptor', 'k1');\r\n```\r\nThis is also allowed, but dumb:\r\n```\r\nm = AddReaction(m, {'glucose', 'membrane.receptor'}, 'membrane.glucose:receptor', 'k1', 'ecm');\r\n```\r\n\"Transport\" and reactions \"inside\" the cell will be simpler:\r\n```\r\nm = AddReaction(m, 'ecm.glucose', 'glucose', 'k2');\r\nm = AddReaction(m, 'ecm.glucose', 'cell.glucose', 'k2');\r\nm = AddReaction(m, 'glucose', 'G6P', 'k3');\r\n```\r\nAgain, this is allowed, but dumb:\r\n```\r\nm = AddReaction(m, 'glucose', 'cell.glucose', 'k2', 'ecm');\r\n```\r\nYes, this is a lot of flexibility, but I think the intent is clear in all cases. To limit flexibility, you could remove the \"default compartment for reaction\" argument and require all species not in the global default compartment to be prefixed.\r\n\r\n##### Implementation vs interfaces\r\nThis is probably the most important underlying theme behind our apparent differences in philosophies. \r\n> Everywhere you use a species, you have to write 'extracellular.glucose' which is the same amount of typing, but much more internally complex, than 'extracellular_glucose'.\r\n\r\n`ecm_glucose` is more *externally* complicated than `ecm.glucose`. The user must manually munge all calls to compartment.species in their workflow. Even if this is automatically done in SBML/whatever -> kroneckerbio conversion code, any analysis must also make these changes on the user's end. Internal complexity can appear at several different levels: the parsing code, or deeper internals like the symbolic solvers, integrators, etc. Proper abstraction via IDs (the existing method) encapsulates this internal complexity in the parser code alone. This leads to the additional points:\r\n\r\n##### Names vs IDs\r\n> Names must be unique model-wide. This is something I think you don't like. But don't tell me this makes things more complicated for the user. If you want to relax this, then you have to make the rules and restrictions on expression strings much more complicated.\r\n\r\nMy preference is that `compartment.species` be unique model-wide to ease organizational burden on the user. kroneckerbio should be flexible enough to respect this. IDs shouldn't normally be visible to the user, which is why I accept them being UUIDs. \r\n\r\nThe rules for valid IDs are not currently enforced in the latter parts of `finalizeModelAnalytic` because they are assumed to be valid when all names are replaced with IDs earlier (the weird substitution error you encountered in issue #44 occurred because you violated this contract). There are currently 2 separate parsing steps when building models, 1 for substituting names (including quoted names w/ invalid characters and compartment.species) with IDs, and 1 for processing expressions with IDs into the ordered `x`'s, `k`'s, `s`'s, etc. that the kroneckerbio internals work with. More formally, in order in `finalizeModelAnalytic`:\r\n1. `name2id` is a lexer that converts \"invalid\" lexemes to \"valid\" tokens. It implements the above rules.\r\n2. `subs` is a parser that takes these results and creates the AST that meaningfully (to mupad) represents the mathematical expressions.\r\n3. `symbolic2function` (and helpers scattered throughout) is a parser that takes these results and creates the functions that meaningfully (to kroneckerbio) represents the states, sensitivities, etc.\r\n\r\nEssentially, `name2id` can always be made better, and much of the functionality described in this section should probably be separated out into a dedicated lexing/parsing module. In fact, we're probably reaching a point where all the parsing/conversion code, including the custom kroneckerbio file format, should be done with formal grammars and proper parser generators. It seems to me that you want to restrict user flexibility for the purpose of making `name2id` simpler.\r\n\r\n> One thing that requires more typing in my design is having to use quotes around names even when the name was a valid identifier.\r\n\r\nToo much work, isn't logically necessary given the rules for names.\r\n\r\n> This is helped by having the default identifier be equal to the name when the name is a valid identifier rather than a UID, which was implemented at the same time.\r\n\r\nSince a mapping always has to be preserved between names and IDs, it's easier to not have to handle this special case (or the converse). We've already discussed the complications surrounding transforming/munging invalid names into valid ones at length. Again, users shouldn't have to see IDs so it shouldn't matter.\r\n\r\n> We probably should add a helper function m = simplifyIDs(m) that purges the Simbiology IDs and replaces them with simpler IDs.\r\n\r\nSame as above. Btw, there's a flag in the SBML importer that deals with SBML models that use names as IDs and nothing as names. The Brown model uses it.\r\n\r\n> Names must not contain double quote characters when used in expressions\r\n\r\nAs an alternative, we could do what SimBiology does and surround invalid names with square brackets `[`, `]`. Don't comment on this, it's trivial. On second thought, it's not trivial, as it's easier to lex the brackets than quotes, looking at the SimBio conversion code.\r\n\r\n##### Separation of concerns\r\n> Analytic models are symbolic models with additional functionality added to them.\r\n\r\nAnalytic models are not symbolic models. Symbolic models (as a proxy for a converter class) interconvert between different model types, including massaction models. Analytic models are fed to an integrator (as a proxy for an integrator being fed into a model class?) to simulate the system. The reason I brought back symbolic models was because of this plus a lot of shared logic between converting between different model types. Except the functions that do symbolic <-> analytic model conversion, the functions that deal with symbolic models shouldn't touch analytic models. \r\n\r\nThis is an organizational issue, though, so I'll have to think more about how the different parts of kroneckerbio interact at a high level. Somewhat related to the modules issue #10, but with much broader scope.\r\n\r\nSomething that will remain useful, though, is for analytic models to maintain their ability to finalize themselves. They should also continue to be analogous to massaction models (which is why I disagree with requiring massaction models to have globally unique species names as well).\r\n\r\n##### Future directions / should probably go into a separate issue\r\nWhat are the things that can be built from analytic models? Right now, I would like a way to build model variants cheaply. Might be incompatible with simulating them in parallel - who knows?\r\n\r\nI think we're really pushing the limits of what we should be doing with closures. Large parts of kroneckerbio are also tightly coupled, as seen from the huge amount of code changed with each of these commits. I'd like some input on refactoring various things into classes, but especially the interfaces between the things in kroneckerbio (not necessarily using classes)."}}