Fix bugs in load_budget
Fixes two bugs in load_budget
- 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.
- 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.
Merge request reports
Activity
added bug label
137 138 return inherit_budget 138 139 else: 139 140 modname = 'gwinc.ifo.aLIGO' 141 bname = bname or 'aLIGO' 142 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 stringBut with inheritance now, yaml files do specify which budget to use if there's a
+inheritkey. 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.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.IFOSif the inheriting yaml file was in an arbitrary directory.The way things currently work is that if you call
load_budgeton a yaml file that does not have a+inheritkey, 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 newtest_load_uninherited_yamlin 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 callStruct.from_fileon a yaml file with a+inheritkey will raise an error currently, though this is something we want to fix in the future.
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.
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,
bnameisNonewhich is no good when callinggetattr(mod, bname)a few lines down.
125 125 if inherit_ifo is not None: 126 126 del ifo['+inherit'] 127 127 # make the inherited path relative to the loaded path 128 # if it is a yml file 129 if os.path.splitext(inherit_ifo)[1] in Struct.STRUCT_EXT: 130 base = os.path.split(path)[0] 131 inherit_ifo = os.path.join(base, inherit_ifo) 128 # if it is a yml file or a directory 129 head = os.path.split(path)[0] 130 rel_path = os.path.join(head, inherit_ifo) 131 if os.path.splitext(inherit_ifo)[1] in Struct.STRUCT_EXT or os.path.exists(rel_path): -
To your broader comment @kevin.kuns, I think we should throw an error here if the ext is not in STRUCT_EXT.
125 125 if inherit_ifo is not None: 126 126 del ifo['+inherit'] 127 127 # make the inherited path relative to the loaded path 128 # if it is a yml file 129 if os.path.splitext(inherit_ifo)[1] in Struct.STRUCT_EXT: 130 base = os.path.split(path)[0] 131 inherit_ifo = os.path.join(base, inherit_ifo) 128 # if it is a yml file or a directory 129 head = os.path.split(path)[0] 130 rel_path = os.path.join(head, inherit_ifo) 131 if os.path.splitext(inherit_ifo)[1] in Struct.STRUCT_EXT or os.path.exists(rel_path): 132 inherit_ifo = rel_path 132 133 133 134 inherit_budget = load_budget(inherit_ifo, freq=freq, bname=bname) added 1 commit
- 5c5c5a96 - check that file types are supported by Struct in load_budget
assigned to @christopher.wipf
@christopher.wipf This is an actual bug. You can ignore my suggestion about loading into the aLIGO budget. But the MR as is only fixes the bugs.
I agree with Jamie's point that the design of the inheritance feature could benefit from more thought. I also agree with Kevin that inheritance design discussion is separable from this MR. Are there are objections to how this MR fixes the problems it sets out to fix? If not, let's plan to merge it later this week.
mentioned in commit 4bd943ee
