Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace use of strings with pathlib #895

Open
carandraug opened this issue Jun 10, 2024 · 11 comments
Open

replace use of strings with pathlib #895

carandraug opened this issue Jun 10, 2024 · 11 comments

Comments

@carandraug
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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

  • empty strings evaluate as false but Path('') does not
  • in experiments we often treat the empty string as "don't read/write" file so this had to change to use None
  • wx does not do pathlib so I'm often converting between string and pathlib (my approach has been to convert to Path as soon as possible and only convert to string when needed for a dependency)
  • pathlib does not expand variables and is completely against it. So in that case in use os.path
  • I'm still using the open function and did not replace it with pathlib.Path.open

Can you try the wip-895-pathlib branch?

@iandobbie
Copy link
Collaborator

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
Copy link
Collaborator

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

   mrcPath = savePath + '.mrc'

cant add a WindowsPath and a str object.

@iandobbie
Copy link
Collaborator

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
Copy link
Collaborator

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

    if savePath.suffix == '.txt':
        mrcPath = savePath.with_suffix('.dv')

@iandobbie
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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

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

No branches or pull requests

2 participants