Closed weiglszonja closed 2 years ago
It shouldn't be necessary and is definitely not a good thing to do - I think this was a quick hack to make the tests work back when the code was first imported over. If you can add some code to ROIExtractors (and tests) that 100% guarantee every sampling frequency from every extractor returns a float, then yes that line will no longer be needed here.
@CodyCBakerPhD How would you do it? I was thinking to add a decorator (yeah, I know...) that checks the return value of the get_sampling_frequency
function:
def check_get_sampling_frequency_return_dtype(func):
""" A decorator to check the return value of a function is of float type. """
@wraps(func)
def wrapper(*args, **kwargs):
return_value = func(*args, **kwargs)
if not isinstance(return_value, float):
return_value = float(return_value)
return return_value
return wrapper
We are already using typehints:
I think we should just ensure that we are following our own typehints and make sure all extractors return the appropriate type in the implementation of those extractors.
@CodyCBakerPhD How would you do it? I was thinking to add a decorator (yeah, I know...)
I would simply write a test on the ROIExtractors side to ensure the output of each extractor .get_sampling_frequency
is a float type.
The conversion tools shouldn't do too much magic like this on their own, they should ideally just expect particular forms of inptus/outputs and not behave correctly if that is violated. (the standard implication of type hints)
I did look into Pydantic type stuff for this a while back but the huge downside to that is they are not compatible with numpy data typing, and complicated unions of things can throw it off.
yes, well put, Cody. Magic like this is difficult to maintain and tests are much simpler and just as effective.
The only place in the pipeline where we need this was changed recently and now does a casting over there: https://github.com/catalystneuro/nwb-conversion-tools/blob/07e958d7564eb12d00a5ff5fabe661677d023efa/src/nwb_conversion_tools/tools/roiextractors/roiextractors.py#L110
I just tested removing the line in the metadata and it works as it is.
Concerning checking for sampling rate equal being a float in imaging extractor, check: https://github.com/catalystneuro/roiextractors/pull/164
This is removed now in #559. Should we close the issue, @weiglszonja or is there something missing?
Yeah its fine, I’ll close it.
Accessing the private attribute sampling frequency and casting to float in
get_metadata_schema
is not a proper place to have anymore, I think it should be removed. https://github.com/catalystneuro/nwb-conversion-tools/blob/07e958d7564eb12d00a5ff5fabe661677d023efa/src/nwb_conversion_tools/datainterfaces/ophys/baseimagingextractorinterface.py#L27-L29@CodyCBakerPhD what do you think?