Skip to content

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

JoeZiminski
Copy link
Member

@JoeZiminski JoeZiminski commented May 8, 2025

This PR extends the narrow datatypes to include motion and emg, 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.

Comment on lines +376 to +381
is_not_missing_narrow_datatypes = all(
[
dtype in all_narrow_datatypes
for dtype in user_settings["tui"]["create_checkboxes_on"]
]
)
Copy link
Collaborator

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

Comment on lines +384 to +389
assert all(
[
dtype in all_narrow_datatypes
for dtype in user_settings["tui"]["transfer_checkboxes_on"]
]
)
Copy link
Collaborator

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]
Copy link
Collaborator

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.

Comment on lines +420 to +427
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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants