peng-lab / BaSiCPy

MIT License
60 stars 20 forks source link

Fix type validation for BaSiC autosegment field #137

Closed carshadi closed 9 months ago

carshadi commented 9 months ago

Currently, passing a callable to the autosegment field of BaSiC results in the following exception:

ValidationError: 1 validation error for BaSiC
autosegment
  value could not be parsed to a boolean (type=type_error.bool)

Since the documentation states this field supports either a bool or callable, i'm guessing this is a bug.

This PR changes the validation to accept either bool or a callable which accepts and returns a single numpy array

yfukai commented 9 months ago

Hi, thanks @carshadi, I believe this is a bug in documentation, since I removed the callable support here but forgot to modify the documentation. https://github.com/peng-lab/BaSiCPy/pull/130/commits/c11b073de6670eac619bcf4c911251716e826dd1

I don't remember the reason for this, but if you're interested in this feature you could try to re-implement this and see how it works.

carshadi commented 9 months ago

Ah, I see. I figured it was still supported since a callable is used here if provided https://github.com/peng-lab/BaSiCPy/blob/07d438ed58b8a9ab154a41a4d57e44de6b631fe6/src/basicpy/basicpy.py#L267-L278

It does seem like it could be a useful feature to bring back. In our case the otsu threshold tends to be a bit too conservative, only capturing a small fraction of the background voxels. The idea is to segment the background voxels and only run the optimization on those, correct? I will experiment more with this in a fork and get back to you.

Thanks for your help!

yfukai commented 9 months ago

Oh yeah right, I'm sorry for the inconsistency. I believe I removed the support because of an error at the model saving, so maybe we need some workaround for that (or simply it can emit error when the parameter is callable). Thank you for your interest on reimplementing this!

https://github.com/peng-lab/BaSiCPy/blob/07d438ed58b8a9ab154a41a4d57e44de6b631fe6/src/basicpy/basicpy.py#L855

The idea is to segment the background voxels and only run the optimization on those, correct?

Yes, definitely. Maybe you're aware of, but let me mention you can supply the fitting routine with the fitting_weight parameter if you would like to use your own segmentation.

https://github.com/peng-lab/BaSiCPy/blob/07d438ed58b8a9ab154a41a4d57e44de6b631fe6/src/basicpy/basicpy.py#L283

carshadi commented 9 months ago

I see, I had missed the fitting_weight option somehow. That would be all I need. Will close this. Thanks!