76 overhaul json parsing approach #91
Conversation
There was a problem hiding this comment.
A really nice refactor, a lot was simplified and made much easier to read here. Having a single base class for components is a great move. I think we may just need a test adding to cover the functionality previously covered by test_make_everything. We may also want to consider improving the efficiency of the recursive for loop, but that could be a separate issue.
| def test_make_everything(): | ||
| # this should make a pin assembly | ||
| geom_list = make_everything(PIN) | ||
| geom = geom_list[0] | ||
| assert len(geom_list) == 1 | ||
| assert isinstance(geom, PinAssembly) | ||
|
|
||
| # make sure it contains all the components we want | ||
| comp_classes = [type(comp) for comp in geom.components] | ||
| for pin_comp in PIN_COMPS: | ||
| assert pin_comp in comp_classes | ||
|
|
||
| fake_json_obj = [PIN] | ||
| json_list = make_everything(fake_json_obj) | ||
| assert len(json_list) == 1 | ||
| assert isinstance(json_list[0], PinAssembly) | ||
|
|
||
| with pytest.raises(CubismError): | ||
| make_everything(1) | ||
|
|
||
|
|
There was a problem hiding this comment.
Do we lose any coverage by removing this? If so, could we add a test for the new approach?
There was a problem hiding this comment.
This should have only been testing for whether that function worked as intended. Geometry creation is now being tested for (implicitly?) when using GeometryMakers make_geometry so I think we should be fine?
| # it take so long to run :( | ||
| def classdict(cls): | ||
| '''Find all subclasses recursively. | ||
|
|
||
| Returns | ||
| ------- | ||
| dict | ||
| {"Class": Class} | ||
| ''' | ||
| return_dict = {} | ||
| for subcls in cls.__subclasses__(): | ||
| return_dict[subcls.__name__] = subcls | ||
| return_dict.update(classdict(subcls)) | ||
| return return_dict |
There was a problem hiding this comment.
If this is taking a long time to run, maybe let's put in an issue to consider ways to make it more efficient. Python for loops are notoriously slow, but that said it's unlikely to be the bottleneck in the high fidelity simulation workflows hypnos is supporting.
There was a problem hiding this comment.
I'm not actually sure it's that slow, it just feels inefficient. Will add an issue just to keep note though.
The aim here was to remove dependence on globals() so we can stop caring about scope.
i have done this by adding a classdict() function that queries a class for all it's subclasses.
CLASS_MAPPING has also been removed, so classes must be requested using their python names
New
Updated
Removed