Skip to content

Fix for issue #466#471

Closed
aakash-kharb wants to merge 1 commit intoneuroinformatics-unit:mainfrom
aakash-kharb:Aakash/466
Closed

Fix for issue #466#471
aakash-kharb wants to merge 1 commit intoneuroinformatics-unit:mainfrom
aakash-kharb:Aakash/466

Conversation

@aakash-kharb
Copy link

Enhacement for issue #466 . Create a new function to check for datatype mismatch between narrow and broad datatypes.

@aakash-kharb
Copy link
Author

Thanks a lot @parharti and @JoeZiminski . Sorry, had to revamp through a new fork . But here I have split commits for both issues in separate PR's

@aakash-kharb
Copy link
Author

I have your comments from the previous PR, I'll be going through them and continue with the refactoring and all. Thanks for the feedback.

@JoeZiminski
Copy link
Member

Hi @aakash-test7 thanks a lot for this, much appreciated. I will review once #464 is merged, because I think this PR can leverage some of the new changes. Cheers!

@JoeZiminski
Copy link
Member

Hi @aakash-test7 thanks for this, I can see the logic of testing the broad / narrow datatypes are not mixed, and is a very useful endeavour #466. In this case, the datatype attribute on a config dict is checked, but the config dictionary does not have a datatype key.

Typically the broad / narrow datatype will need to be checked in two places. Firstly, when the user passes a list of datatypes with create_folders and secondly, when validating a project. The first is not too bad because it is based on a list of passed names. The second is when validating across local and central projects. This case is trickier (it would need to extend this function) and would need to be done within the new strict_mode. A list of all datatypes used would need to be tracked across local / central projects and then validated. This will get quite complex and would probably require a lot of refactoring so is not a high priority for now.

In terms of the validation, given a list of datatypes I think the checking logic could be something like:

if any([dtype in narrow_dtypes for dtype in list_of_dtypes]):
    assert not any([dtype in broad_dtypes for dtype in list_of_dtypes])

This is not very efficient as it does two full passes through the list but this list is very unlikely to be bigger than 1000 elements, 10,000 in a test case took 0.0008 s so I don't think we need to prioritise efficiency here.

I will close this for now but thanks a lot for taking a look at this issue!

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