-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add default arguments ability and update tests. #91
Conversation
|
Having thought about it, I don't think that it is sensible to have a default 'handle'. It makes sense to want to work in engineering or physics units, or to work with the sim or live, but not to work with 'setpoint' all the time. I suggest we remove that aspect from the pull request. |
pytac/__init__.py
Outdated
ENG = 'engineering' | ||
PHYS = 'physics' | ||
# Data Source types. | ||
SIM = 'simulation' | ||
LIVE = 'live' | ||
# Default argument flag. | ||
default = 'default' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a constant so should be in all caps i.e.
DEFAULT = 'default'
Ok I will remove default handle and put it back to how it was with get_value using pytac.RB as the default specified argument and set_value using pytac.SP. |
pytac/data_source.py
Outdated
units = self._default_units | ||
if data_source is pytac.default: | ||
if data_source is pytac.DEFAULT: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should really use ==
not is
in this case. I think it's unlikely to cause you a problem, but there's a chance that you end up with a string default
that won't compare correctly as DEFAULT
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That stack overflow makes sense, interestingly I currently can't cause that problem in our code, but I will change it just in case.
…t for element creation and update tests.
pytac/lattice.py
Outdated
element.set_value(field, value) | ||
element.set_value(field, value, handle=pytac.SP) | ||
|
||
def set_default_arguments(self, default_units=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It now looks a bit strange for this to be one method - I suggest you split it into two.
pytac/lattice.py
Outdated
raise LatticeException('Please set at least one default argument ' | ||
'for units or data_source.') | ||
if default_units == pytac.ENG or default_units == pytac.PHYS: | ||
self._data_source_manager._default_units = default_units |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're using a private attribute _default_units
from outside the DataSourceManager class. Should it be a public attribute?
pytac/lattice.py
Outdated
return self._data_source_manager.default_units | ||
|
||
def get_default_data_source(self): | ||
"""Get the default data source, pytac.RB or pytac.SP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment wrong.
pytac/lattice.py
Outdated
pytac.LIVE, pytac.SIM)) | ||
|
||
def get_default_units(self): | ||
"""Get the default unit type, pytac.RB or pytac.SP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment wrong.
I'm finding it quite hard to think through all the consequences of these changes. I've made a couple of comments about the docstrings, then I'll merge and we'll see how we get on. |
Fixed those two mistakes. I'm not sure if there are many consequences, as pytac will function as it does currently if the defaults are not changed and I don't think there are any complications when defaults are set as the use cases are pretty limited. |
Changes made - ready to merge again.
I also added tests for the template classes Device and DataSource, as ControlSystem already had them.
So I have renamed test_cs to test_invalid_classes, all that it does is checks all methods on all the template classes raise NotImplementedError.