Refact component type #726
Conversation
luojing1211
requested review from
demorest,
aarchiba,
mtlam,
paulray and
scottransom
luojing1211
force-pushed
the
refact_component_type
branch
from
c7c07bc
to
8f37f0b
Compare
|
Hi all, could you please take a look at this PR. I would like to move it forward for the wideband TOA development. |
| sector = DelaySector(self.all_components["AstrometryEquatorial"]()) | ||
| copy_sector = deepcopy(sector) | ||
|
|
||
| assert id(sector) != id(copy_sector) |
There was a problem hiding this comment.
What is this testing? Whether copy.deepcopy works?
| assert sector.__class__.__name__ == "DelaySector" | ||
| assert len(sector.component_list) == 1 | ||
| assert hasattr(sector, "delay") | ||
| assert hasattr(sector, "delay_funcs") |
There was a problem hiding this comment.
I'm not really clear what the sector functions do but this seems like it's not testing very much - I'm not clear what aspects it is actually testing?
| assert len(tm.DelayComponent_list) == 4 | ||
| assert len(tm.PhaseComponent_list) == 2 | ||
| assert len(tm.delay_components) == 4 | ||
| assert len(tm.phase_components) == 2 |
There was a problem hiding this comment.
This renaming seems like a definite improvement. But where are the tests of the new functionality coming from this change?
| ---------- | ||
| param: str | ||
| Parameter name | ||
| target_component: str |
There was a problem hiding this comment.
Should this accept a Component object as well as the name of a Component?
| @@ -181,3 +181,7 @@ for comp, tp in Component.component_types.items(): | |||
|
|
|||
| special | |||
| ``` | |||
|
|
|||
| ```python | |||
| """ Class for holding all delay components and their APIs | ||
| """ | ||
|
|
||
| _methods = ( |
There was a problem hiding this comment.
It looks like some of these are methods and others are properties?
| @@ -1391,3 +1392,38 @@ def FTest(chi2_1, dof_1, chi2_2, dof_2): | |||
| log.warning("Models have equal degrees of freedom, cannot preform F-test.") | |||
| ft = False | |||
| return ft | |||
|
|
|||
|
|
|||
| def get_component_type(component): | |||
There was a problem hiding this comment.
Does this return the actual type object? Is it "type" in the python sense you mean here?
| Object --> component --> TypeComponent. (i.e. DelayComponent) | ||
| This class type is in the third to the last of the getmro returned | ||
| result. | ||
|
|
There was a problem hiding this comment.
No need for blank lines at the end of a docstring.
| @@ -50,7 +51,7 @@ def test_component_categories(model): | |||
|
|
|||
| """ | |||
| for k, v in model.components.items(): | |||
| assert model.get_component_type(v) != v.category | |||
| assert get_component_type(v) != v.category | |||
There was a problem hiding this comment.
This makes it look like here "type" means the same thing as "category"?
| ) | ||
|
|
||
|
|
||
| class TestSector: |
There was a problem hiding this comment.
It seems worth checking that the code correctly rejects sectors that share "methods", that all listed methods in existing sectors actually exist, and that the "external_sector_map" actually does what it's supposed to.
This Pull Request re-facts the way TimingModel stores its components and the APIs to gather the information from the same type of components. The user interface has changed as little as possible.
Motivation:
When developing Wideband TOAs, we are adding several components: DMJumps, DM noises. The current TimingModel is too big and not modulized enough.
Things are changed:
Benefits: