SNEWS2 / snewpy

A Python package for working with supernova neutrinos
https://snewpy.readthedocs.io
BSD 3-Clause "New" or "Revised" License
27 stars 22 forks source link

Fix Windows-specific bugs #341

Closed JostMigenda closed 4 months ago

JostMigenda commented 6 months ago

This should eventually fix #340.

JostMigenda commented 6 months ago

The first commit (50ba643) adds test for Windows[*] without any fixes to the code; to confirm that the tests fail in CI and reproduce the issue observed by our project student.

The second commit (22b66ce) adds a fix for the Warren_2020 model download, as described in #340. This fixes the Warren_2020 test failures, as expected.

Now the only remaining test failure is in test_flux_container.py:115 (test_save_and_load), which has a hard-coded file path that doesn’t appear to work under Windows: FileNotFoundError: [Errno 2] No such file or directory: '/tmp/flux.npz' I’ll check if there’s an easy fix for this.

[*] and MacOS, for good measure; though that is a UNIX and shouldn’t have as many peculiarities

JostMigenda commented 6 months ago

… and commit 4bb76d5 replaces the hardcoded path by using the tempfile module; so it now works on Windows. This is now ready for review.

JostMigenda commented 6 months ago

Hm … one remaining issue in the integration tests: The command jupyter execute ccsn/*.ipynb fails on Windows (see lines 136–156 of the log) (and same for presn), but the return code is still successful.

I see a few options:

  1. Figure out how to implement that wildcard correctly in a Windows shell
  2. Skip that step on Windows
  3. Ignore; the other Jupyter notebooks in the step are running successfully, so this is still a useful test

Option 1 would be the right thing to do; but since I don’t have access to a Windows machine, that doesn’t sound like a rabbit hole I particularly want to go down … 🫣

JostMigenda commented 6 months ago

After some experimentation, it turns out that PowerShell apparently does not expand wildcard characters (which is not obvious from the documentation 🤬). Commit c713c78 adds the additional code required for this. With that, afaict the test on Windows are equivalent to those on Linux/MacOS and this PR is ready for review.