search

owkin / HistoSSLscaling

Code associated to the publication: Scaling self-supervised learning for histopathology with masked image modeling, A. Filiot et al., MedRxiv (2023). We publicly release Phikon 🚀
Other
142 stars 11 forks source link

Update chowder.py #19

Closed VolodymyrChapman closed 9 months ago

VolodymyrChapman commented 9 months ago

Add metadata_cols argument to Chowder class to allow for input of data with preceding columns (for example, magnification) to be ignored by the chowder model. Default of 3 assumes magnification, start_x and start_y columns preceding data to be fitted / inferred on, in keeping with current behaviour of 3 preceding columns - i.e. default settings will not break behaviour of existing Chowder models / code. Testing on fresh conda environment with modified Chowder model:

image

jbschiratti commented 9 months ago

Thanks @VolodymyrChapman for initiating this PR.

To keep a consistent behavior, can you also add the metadata_cols parameter in other MIL models?

VolodymyrChapman commented 9 months ago

@jbschiratti Of course - will do :-)

jbschiratti commented 9 months ago

Thanks @VolodymyrChapman!

Although there's no CI (yet) on this repository, can you, please, update the unit tests and make sure they pass locally?

jbschiratti commented 9 months ago

@VolodymyrChapman Looks like you pushed some unnecessary .pyc files by mistake. Can you please remove them from this PR?

VolodymyrChapman commented 9 months ago

@jbschiratti Of course :-) The metadata_cols variable has been added to all MIL models, as per the Chowder model. The variable has also been added to all models in the tests/models/test_slide_models.py file. Linting was performed with black using the specifications outlined in dev_tools/linting.sh Local output of black checks and unittest for just the MIL models (Ubuntu 22.04 in a fresh conda env): image

Are there any other tests / checks that you would require?

jbschiratti commented 9 months ago

@VolodymyrChapman Once you've removed the .pyc files, this will be good to go.

I'll let @afilt take over as he's the code owner on this repository.

VolodymyrChapman commented 9 months ago

@jbschiratti Thank you for spotting - apologies for the spam files! I think they've all been removed now.

VolodymyrChapman commented 9 months ago

@afilt Thank you! I don't think I have necessary write access to perform the merge as the button is missing from my view: image Please let me know if the merge is successful from your end so I can delete the fork :-)

afilt commented 9 months ago

You're right :-) Thanks a lot @VolodymyrChapman !

VolodymyrChapman commented 9 months ago

My pleasure - happy to help :-)