ModelSEED / ProbModelSEED

Other
2 stars 3 forks source link

object_refs have vertical bars #16

Open nconrad opened 9 years ago

nconrad commented 9 years ago

Just pointing out that there are odd ||'s in media_ref and the modelcompound_ref's for fba data. This doesn't affect my work.

I still wonder why this data needs to be returned as a string. We should fix that later on if possible.

{
    "meta": [
        "fba.23",
        "fba",
        "/nconrad@patricbrc.org/home/models/.testgenome1.genome_model/fba/",
        "2015-06-04T23:08:47",
        "A7854650-0B0E-11E5-9AEC-C3F8682E0674",
        "nconrad@patricbrc.org",
        15047,
        {
            "media": "/chenry/public/modelsupport/media/Complete",
            "objective": 0
        },
        {
            "objective_function": "Max bio1",
            "allReversible": 0,
            "findMinimalMedia": 0,
            "model": "/nconrad@patricbrc.org/home/models/testgenome1.genome_model",
            "simpleThermoConstraints": 0,
            "objectiveValue": 0,
            "comboDeletions": 0,
            "media": "/chenry/public/modelsupport/media/Complete",
            "fva": 0,
            "objectiveConstraintFraction": 1,
            "fluxMinimization": 0,
            "no_production_biomass_compounds": "cpd00003_c0/cpd00006_c0/cpd00016_c0/cpd15500_c0/cpd00220_c0/cpd15723_c0/cpd00052_c0/cpd00038_c0/cpd02229_c0/cpd15665_c0/cpd15560_c0/cpd15352_c0/cpd00357_c0/cpd00087_c0/cpd00015_c0/cpd00345_c0/cpd15750_c0/cpd15768_c0/cpd00062_c0/cpd00201_c0/cpd15722_c0/cpd15794_c0/cpd15795_c0/cpd00356_c0/cpd15748_c0/cpd15766_c0/cpd00028_c0/cpd15696_c0/cpd15695_c0/cpd15533_c0/cpd15749_c0/cpd15767_c0/cpd15540_c0/cpd15793_c0/cpd00241_c0/cpd15669_c0/cpd15757_c0/cpd15775_c0/cpd11459_c0/cpd15758_c0/cpd15667_c0/cpd15668_c0/cpd15776_c0/cpd15777_c0/cpd15759_c0/cpd00063_c0",
            "id": "fba.23",
            "thermodynamicConstraints": 0
        },
        "o",
        "n",
        ""
    ],
    "data": {
        "compoundflux_objterms": {},
        "media_ref": "/chenry/public/modelsupport/media/Complete||",
        "FBAMetaboliteProductionResults": [
            {
                "modelcompound_ref": "/nconrad@patricbrc.org/home/models/testgenome1.genome_model||/modelcompounds/id/cpd00161_c0",
                "maximumProduction": 100
            },
            {
                "modelcompound_ref": "/nconrad@patricbrc.org/home/models/testgenome1.genome_model||/modelcompounds/id/cpd00033_c0",
                "maximumProduction": 100
            },
            {
                "modelcompound_ref": "/nconrad@patricbrc.org/home/models/testgenome1.genome_model||/modelcompounds/id/cpd00002_c0",
                "maximumProduction": 100
            },
            {
                "modelcompound_ref": "/nconrad@patricbrc.org/home/models/testgenome1.genome_model||/modelcompounds/id/cpd00010_c0",
                "maximumProduction": 100
            },
            {
                "modelcompound_ref": "/nconrad@patricbrc.org/home/models/testgenome1.genome_model||/modelcompounds/id/cpd00051_c0",
                "maximumProduction": 100
            },
            {
                "modelcompound_ref": "/nconrad@patricbrc.org/home/models/testgenome1.genome_model||/modelcompounds/id/cpd00001_c0",
                "maximumProduction": 100
            },
            {
                "modelcompound_ref": "/nconrad@patricbrc.org/home/models/testgenome1.genome_model||/modelcompounds/id/cpd00003_c0",
                "maximumProduction": -1.13687e-13
            },
            {
                "modelcompound_ref": "/nconrad@patricbrc.org/home/models/testgenome1.genome_model||/modelcompounds/id/cpd00060_c0",
                "maximumProduction": 100
            },
            {
                "modelcompound_ref": "/nconrad@patricbrc.org/home/models/testgenome1.genome_model||/modelcompounds/id/cpd00035_c0",
                "maximumProduction": 100
            },
            {
                "modelcompound_ref": "/nconrad@patricbrc.org/home/models/testgenome1.genome_model||/modelcompounds/id/cpd11493_c0",
                "maximumProduction": 100
            },
            {
                "modelcompound_ref": "/nconrad@patricbrc.org/home/models/testgenome1.genome_model||/modelcompounds/id/cpd00006_c0",
                "maximumProduction": 6.82121e-13
            },
            {
                "modelcompound_ref": "/nconrad@patricbrc.org/home/models/testgenome1.genome_model||/modelcompounds/id/cpd00016_c0",
                "maximumProduction": 0
            },
            {
                "modelcompound_ref": "/nconrad@patricbrc.org/home/models/testgenome1.genome_model||/modelcompounds/id/cpd00156_c0",
                "maximumProduction": 100
            },
            {
                "modelcompound_ref": "/nconrad@patricbrc.org/home/models/testgenome1.genome_model||/modelcompounds/id/cpd00254_c0",
                "maximumProduction": 100
            }
.
.
.
mmundy42 commented 9 years ago

Chris, I don't quite understand the "||" notation either. There is magic in PATRICStore.pm that updates object references from UUIDs to workspace paths (for example https://github.com/ModelSEED/ProbModelSEED/blob/master/lib/Bio/KBase/ObjectAPI/PATRICStore.pm#L133). With the extra "||" the references don't work with the workspace service.

Is there something else going on here that I don't understand?

cshenry commented 9 years ago

The �||� is unavoidable. The problem is that the PATRIC workspace has subdirectories. So workspace paths contain an unpredictable number of �/�, with no clear structure like the KBase workspace had. But we use �/� to denote paths within an object as well. I need to separate workspace paths (e.g. /chenry/home/models/MyModel) from paths to navigate within an object to get to a specific sub object (e.g. reactions/id/rxn00001). To do this, I introduced the �||� as a delimiter between workspace path and object path. I don�t know what else to do. The other possibility is to rethink how we handle refs, and I�m open to that� but it�s a major change and requires careful thought.

On Jun 5, 2015, at 10:11 AM, Mike Mundy notifications@github.com wrote:

er

mmundy42 commented 9 years ago

Ok, that makes sense and it would be a lot of work to change. Although I'm still concerned about a consumer of an object handling the references correctly. Maybe just need to document it.

nconrad commented 9 years ago

Not too familiar with the design, but I'm concerned about this long term. Seems that they should be separate keys then, i.e., ws_path and obj_path

Also, does this matter?

screen shot 2015-06-05 at 2 08 35 pm

mmundy42 commented 9 years ago

I'm guessing that you used a workspace method to create an object with the "||" special token? If so, that would cause all sorts of trouble in the modeling code.

Chris, any thoughts on how big a change it would be to separate the reference to the object from the reference to the subobject inside of it?

cshenry commented 9 years ago

It�s alot of work. Any munging with the refs will impact the very core of the system. Also, refs must be strings� at least right now. I�m game to make changes, but not for the current release.

I know trouble will be caused if someone names an object with a �||�, but this is unlikely to happen often if at all. For now, I�d like to leave as is� We will fix this at some point.

On Jun 5, 2015, at 2:27 PM, Mike Mundy notifications@github.com wrote:

I'm guessing that you used a workspace method to create an object with the "||" special token? If so, that would cause all sorts of trouble in the modeling code.

Chris, any thoughts on how big a change it would be to separate the reference to the object from the reference to the subobject inside of it?

� Reply to this email directly or view it on GitHub.

nconrad commented 9 years ago

That's completely understandable. We could mark some of these tickets toward milestones after this release.

mmundy42 commented 9 years ago

Here are some notes for the future. The objects in KBase::ObjectAPI are still using UUIDs for the references. For example, https://github.com/ModelSEED/ProbModelSEED/blob/master/lib/Bio/KBase/ObjectAPI/KBaseFBA/DB/FBA.pm#L112.

The KBaseStore and PATRICStore objects each have a get_objects() method that transform the _reference field in an object to the appropriate reference for the underlying object store. Maybe there should be a reference object that manages references and could be swapped based on the underlying object store.

nconrad commented 9 years ago

Leaving this here as reminder.

screen shot 2015-06-23 at 6 10 41 pm