-
Notifications
You must be signed in to change notification settings - Fork 22
Add motion
and emg
datatypes
#515
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
base: main
Are you sure you want to change the base?
Conversation
is_not_missing_narrow_datatypes = all( | ||
[ | ||
dtype in all_narrow_datatypes | ||
for dtype in user_settings["tui"]["create_checkboxes_on"] | ||
] | ||
) |
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.
a subtle bug here. it should probably be something
is_not_missing_narrow_datatypes = all(
[
dtype in user_settings["tui"]["create_checkboxes_on"]
for dtype in all_narrow_datatypes
]
)
if we're checking for all narrow datatype to be present in user_settings
assert all( | ||
[ | ||
dtype in all_narrow_datatypes | ||
for dtype in user_settings["tui"]["transfer_checkboxes_on"] | ||
] | ||
) |
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.
same here
"create_checkboxes_on" | ||
][key] | ||
new_transfer_checkbox_configs[key]["on"] = settings["tui"][ | ||
new_transfer_checkbox_configs[key]["on"] = user_settings["tui"][ | ||
"transfer_checkboxes_on" | ||
][key] |
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.
I am not sure but atm, this can break things now especially when we're adding more narrow datatypes. There are going to be cases which although use the newer version of datashuttle i.e has_narrow_datatypes
is True
but is_not_missing_narrow_datatypes
is False
(because of "motion" and "emg"), in that case, user_settings["tui"]["transfer_checkboxes_on"][key]
is still a dict but we're assigning it to a boolean value, new_transfer_checkbox_configs[key]["on"]
So, I'm assuming the has_narrow_datatypes
flag can provide us enough information about where user_settings["tui"]["transfer_checkboxes_on"][key]
is a dict (newer datashuttle version ) or not (older datashuttle version). We can then do the assignment according to the flag.
for narrow_datatype in all_narrow_datatypes: | ||
if ( | ||
narrow_datatype | ||
not in user_settings["tui"]["create_checkboxes_on"].keys() | ||
): | ||
user_settings["tui"][ | ||
"create_checkboxes_on" | ||
] = new_create_checkbox_configs |
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.
If this part of the code is intended to copy the user_settings' narow datatypes config to the new_create_checkbox_configs, we should probably do it something like
for narrow_datatype in all_narrow_datatypes:
if (
narrow_datatype
in user_settings["tui"]["create_checkboxes_on"].keys()
):
new_create_checkbox_configs[narrow_datatype] = user_settings["tui"][
"create_checkboxes_on"
][narrow_datatype]
same we can do for transfer_checkboxes_on
below and at the end we can do
user_settings["tui"]["create_checkboxes_on"] = new_create_checkbox_configs
user_settings["tui"]["transfer_checkboxes_on"] = new_transfer_checkbox_configs
This will both maintain the order of key-value pairs in the settings and also ensure that previously set settings are respected.
This PR extends the narrow datatypes to include
motion
andemg
, as per NeuroBlueprint PRs #67 and #68.It required extending the backwards-compatibility checking for the TUI arguments, to fill-in the settings for missing datatypes. In general, the narrow-datatype machinery is not very extensible, this should be addressed in issue #516.