Fix bugs in load_budget

Fixes two bugs in load_budget

1. Inheritance now works when inheriting a custom budget that is not one of the canonical budgets if the yaml file inheriting it is not in the same directory as the custom budget.
2. According to the help and readme, a yaml file which does not specify a budget to inherit should be loaded into the aLIGO budget. A bug in the logic was causing this to raise an error but is now working again. However, now that inheritance is working I propose that it no longer be possible to load a yaml file which doesn't specify which budget to use.

gwinc/__init__.py

@@ -125,10 +125,11 @@ def load_budget(name_or_path, freq=None, bname=None):
             if inherit_ifo is not None:
                 del ifo['+inherit']
                 # make the inherited path relative to the loaded path
-                # if it is a yml file
-                if os.path.splitext(inherit_ifo)[1] in Struct.STRUCT_EXT:
-                    base = os.path.split(path)[0]
-                    inherit_ifo = os.path.join(base, inherit_ifo)
+                # if it is a yml file or a directory
+                head = os.path.split(path)[0]
+                rel_path = os.path.join(head, inherit_ifo)
+                if os.path.splitext(inherit_ifo)[1] in Struct.STRUCT_EXT or os.path.exists(rel_path):
+                    inherit_ifo = rel_path
 
                 inherit_budget = load_budget(inherit_ifo, freq=freq, bname=bname)
                 pre_ifo = inherit_budget.ifo
@@ -137,10 +138,17 @@ def load_budget(name_or_path, freq=None, bname=None):
                 return inherit_budget
             else:
                 modname = 'gwinc.ifo.aLIGO'
-        else:
+                bname = bname or 'aLIGO'
+
+        elif ext == '':
             bname = bname or base
             modname = path
 
+        else:
+            raise RuntimeError(
+                "Unknown file type: {} (supported types: {}).".format(
+                    ext, Struct.STRUCT_EXT))
+
     else:
         if name_or_path not in IFOS:
             raise RuntimeError("Unknown IFO '{}' (available IFOs: {}).".format(

[Jameson Rollins]

To your broader comment @kevin.kuns, I think we should throw an error here if the ext is not in STRUCT_EXT.

[Kevin Kuns]

That would mean you can't run a custom budget like

python -m gwinc MyBudget

and can only run either an ifo in gwinc.IFOS or a yaml file like

python -m gwinc myifo.yaml

[Kevin Kuns]

Well maybe you mean throw an error if ext isn't in STRUCT_EXT and is also not ''.

[Jameson Rollins]

In fact, I think we should not be using load_budget for handling the inheritance, and in fact not be handling any of this here at all but should instead be handling it in the struct load class methods.

[Kevin Kuns]

Well that is a discussion that we should have, but this MR is fixing a bug in how it is currently supposed to work.

[Kevin Kuns]

I propose that it no longer be possible to load a yaml file which doesn't specify which budget to use. This case should then raise an error instead.

[Jameson Rollins]

The yaml files have never specified which model to use. It's always been the other way around: the budget specifies the yaml (or more precisely, the yaml is usually a part of the budget).

[Kevin Kuns]

I know, but without the line I added you get the error

line 158, in load_budget
    Budget = getattr(mod, bname)
TypeError: getattr(): attribute name must be string

But with inheritance now, yaml files do specify which budget to use if there's a +inherit key. My point is that I don't like that loading a yaml file automatically loads it into the aLIGO budget if nothing is specified. Now that we can, I think it should be unambiguous what is being done.

[Jameson Rollins]

The inherit key specifies a different yaml file to load, not which budget it should be associated with.

[Kevin Kuns]

Not true, yet. If you look at the yaml files in the test/inherit directory, for example, Aplus_mod.yaml loads the ifo.yaml file associated with the Aplus budget into a struct, makes the changes specified, and then initializes the Aplus budget with the modified struct. Aplus_mod2.yaml just inherits the modifications specified by Aplus_mod.yaml. There will always be a budget at the end of the chain of inheritances.

The first bug that's fixed in this MR is that this chain of inheritances didn't work if the base budget was a custom budget not in gwinc.IFOS if the inheriting yaml file was in an arbitrary directory.

The way things currently work is that if you call load_budget on a yaml file that does not have a +inherit key, it initializes the aLIGO budget with the ifo struct specified by the yaml file. This kind of thing can lead to weird bugs and I would prefer that you have to specify which budget is being initialized. This is the functionality that is tested with the new test_load_uninherited_yaml in this MR, but my proposal is that this should raise an error instead.

To be clear, I am not proposing any changes to Struct.from_file. Note also that trying to call Struct.from_file on a yaml file with a +inherit key will raise an error currently, though this is something we want to fix in the future.

[Jameson Rollins]

I don't see what the problem is with assuming the aLIGO budget if no budget is specified. It's currently possible to pass a yaml file on it's own to the CLI and have it do something sensible by assuming the aLIGO budget. I think we should retain that feature.

But I see now the problem you're highlighting, which is that the inheritance assumes you can specify a budget, not a yaml file, to inherit. I think that's the problem, and I think we should enforce that what you're inheriting is another yaml file.

[Kevin Kuns]

I just don't like when things silently default to some option since I've been burned by this several times in the past. But regardless, this change is needed to get your desired behavior. Without it, in this use case, bname is None which is no good when calling getattr(mod, bname) a few lines down.