-
Notifications
You must be signed in to change notification settings - Fork 101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rename DelayComponent_list and PhaseComponent_list? #677
Comments
Hi @paulray. Yeah, I think I agree with you. And that does seem to be something that should probably happen before v0.7 (and be consistent with the paper). I think I'd prefer your last suggestion: .delay_components and .phase_components. |
It is not as trivial as just renaming the list. The name of the list is also used in server other places. For instance, it a component does not belong to any of the current lists (phase, delay, noise), TimingModel will make a new list for it and named str(NewComponentType) + "_list" (see: PINT/src/pint/models/timing_model.py Line 493 in 30d916e
Since the component type is CamelCase, the list ends up like what it looks right now. If we really want to change this, we can add 'delay', 'phase', etc, as a class member to the Component type class. |
I suppose this should be closed since we did not do it before v0.7 and there are some valid reasons for the awkward naming. |
I am working on some fixes to this issue. It will come soon. |
Oops, sorry I thought we decided not to change it. That is great. Reopening. |
It should not be in the 0.7 version milestone. But I realized there would be some roadblock for future development. |
Closed by #726 |
I've been working on the PINT docs, specifically the example notebooks, and I think the names of
TimingModel.DelayComponent_list
andTimingModel.PhaseComponent_list
should be changed. They are the only ones that are in CamelCase and I think it doesn't really make sense.I think we should consider
.delay_component_list
or just.delay_components
.Thoughts @luojing1211 @scottransom @demorest @aarchiba ? If we make this change, we should do it before v0.7 I think.
The text was updated successfully, but these errors were encountered: