search

Jashcraf / poke

Poke (pronounced poh-keh) is a Polarization Ray Tracing and Gaussian Beamlet module for Python
BSD 3-Clause "New" or "Revised" License
33 stars 7 forks source link

Better file organization #98

Open Jashcraf opened 1 year ago

Jashcraf commented 1 year ago

Recently added poke.materials which loads index data in c38ffe6. However the test on GH actions broke because I was supplying it the wrong path? Even though it works locally.

This is what we call in the industry a "skill issue". Maybe this can be @kenjim21's next project

@kenjim21 what do you think of helping me have a better filepath management system that enables better unit tests?

brandondube commented 11 months ago

FWIW, the way I did prysms sample data is invariant to where the code is run

  1. Put the files in a deterministic place that won't change - https://github.com/brandondube/prysm/tree/master/sample_files
  2. Make a small bit of logic that goes and downloads them, if they are not already local, and handles the paths - https://github.com/brandondube/prysm/blob/master/prysm/sample_data.py
  3. Use pathlib because pathlib is love, pathlib is life

The gist of why I did the dynamic download thing is because the sample files are ~10MB, and prysm's source was at the time ~100kb, and I did not want to pollute the source distribution that goes on PyPI.

If your material files are essential, then the dynamic download wouldn't really make much sense (but would allow you to decouple the updating of material data from software versioning, if you wanted to).

What maybe is, or likely is your problem, is a difference in operating system / file system between your local development and the continuous integration (Ubuntu, from a test log).

When you slice up to -12 on line 7 of materials.py, that is potentially error-prone; you could get a trailing slash or not, depending on silly things like non-printing control characters, or double slashes on some platforms, etc. It would be better to write that as Path(__file__).parent/'material_data'). You could also do .parent.absolute() if you want to replicate the absolute portion of that logic, though I think __file__ is always absolute. With os.path, you would want to replace fullpth = matfilepath+'material_data/'+file by fullpth = path.join(matfilepath,'material_data',file). This will let os.path take care of all of the slashes for you.

Basically, don't try to manipulate paths as strings; use a path library (and path lib is the "modern" one that is like 10+ years newer than os.path and less crusty)