microscope-cockpit / cockpit

Cockpit is a microscope graphical user interface. It is a flexible and easy to extend platform aimed at life scientists using bespoke microscopes.
https://microscope-cockpit.org
GNU General Public License v3.0
35 stars 26 forks source link

replace use of strings with pathlib #895

Open carandraug opened 4 weeks ago

carandraug commented 4 weeks ago

Currently we do not use pathlib in cockpit. However, some of our dependencies do. Recently I fixed an issue that was caused because pkg_resources now returns pathlib.Path instances instead of strings which we ten passed to freetype which required a string.

pathlib is now more established and support for it seems to be quite common all over python. We could consider migrating it to.

I do not have a strong opinion --- to be honest I'm not super clear on the advantages of using pathlib ---but if we decide to migrate them we should adopt pathlib everywhere. Having a mix of both seems like the worst case (and I fear that with time we'll be in that situation whether we like it nor not other libraries make that move). Opinions?

iandobbie commented 4 weeks ago

Seems like we are, or are approaching the worst of both worlds with both strings and pathlib being used randomly. Its been in the standard library since 3.4 and might make something easier. I would be inclined to move everything to pathlib, and then call out the exceptions like freetype needing a string.

All that said I haven't even glanced at the code to see how much work this might be. There is a fair amount of code referencing files, config and startup, loading/saving image data, channel configs and I am sure other places too. I will have a look at the code base and see how much work it looks like. We can talk about it at tomorrows development meeting (Note the 30 min late start!)

iandobbie commented 4 weeks ago

A quick grep through the source produces about 300 lines with path in then of which maybe 10% are not about files from a quick glance. So converting from strings seems reasonable but not trivial.

carandraug commented 3 weeks ago

I've spent some time here and here's some notes:

Can you try the wip-895-pathlib branch?

iandobbie commented 3 weeks ago

I pulled it onto my windows machine with picamera and red pitaya executor and it appears to work fine. Opened config files etc. The new display config files worked and it was able to run and save a simple Z stack experiment. Oncxe saved the "view last file" worked fine.

However I ran into trouble in the save mosaic routine. The interface hung with a progress bar saying "Saving mosaic image data" and the stderr had the output "Exception in thread saveTiles" "--- Logging error ---". I will try to find out nmore about what is actually dying.

iandobbie commented 3 weeks ago

The actual crash is in gui/mosaic/canvas.py at line 513

   mrcPath = savePath + '.mrc'

cant add a WindowsPath and a str object.

iandobbie commented 3 weeks ago

The following code fixes the save issue.

       handle = open(savePath, 'w')
        mrcPath = savePath.with_suffix('.mrc')
        if savePath.suffix == '.txt':
            mrcPath = savePath.with_suffix('.dv')
        handle.write("%s\n" % mrcPath)

However, the saved file can now not be loaded, presumable for similar pathlib issues.

iandobbie commented 3 weeks ago

I also don't think we need the code...

    if savePath.suffix == '.txt':
        mrcPath = savePath.with_suffix('.dv')
iandobbie commented 3 weeks ago

Error on load is a dialog saying....

I was unable to load the MRC file at /Users/ID/MUI_DATA/Untitled.dv holding the tile data. The error message was:

'str' object has no attribute 'absolute'

Please verify that the file path is correct and the file is valid.

iandobbie commented 3 weeks ago

Ok this just needs a little edit in the loadtiles function. The following one line change just sets mrcpath to be a pathlib path. It also needs and import at the top of the canvas.py file "from pathlib import Path"

def loadTiles(self, filePath):
    with open(filePath, 'r') as handle:
        mrcPath = Path(handle.readline().strip())
        tileStats = []
iandobbie commented 3 weeks ago

These two changes enable save and reloading of mosaics with the pathlib code.

I also tested the channels load and save which works fine.

iandobbie commented 3 weeks ago

Pushed these changes to my wip-895-pathlib branch on github.