org-arl / arlpy

ARL Python Tools
BSD 3-Clause "New" or "Revised" License
119 stars 37 forks source link

Exposing when `bellhop.exe` is not recognized #103

Open ctuguinay opened 5 months ago

ctuguinay commented 5 months ago

Hey ARL team,

I was trying to use .compute_eigenrays and nothing actually happened (which was indicated by .plot_rays error-ing out), and then I tried again with debug turned on and all I got was:

[DEBUG] Model: bellhop
[WARN] Bellhop did not generate expected output file

This was not too helpful in the debugging process.

I was looking back at the code to see where bellhop.exe is called and I printed out the result of _proc.run of bellhop and got the following:

CODE:

    def _bellhop(self, *args):
        try:
            result = _proc.run(f'bellhop.exe {" ".join(list(args))}',
                            stderr=_proc.STDOUT, stdout=_proc.PIPE,
                            shell=True)
            print(result)

OUTPUT:

CompletedProcess([Some other args here with personal paths].......returncode=1, stdout=b"'bellhop.exe' is not recognized as an internal or external command,\r\noperable program or batch file.\r\n")

With this message, I was able to fix the problem (which ended up being a path issue to bellhop.exe), but I wonder now if stdout when returncode=1 should be exposed to the user during debug is True or just raised as some sort of warning? If any of these sound good, I'd be willing to submit a PR with this proposed change.

Edit: I tested this via the bellhop.ipynb notebook, so perhaps these messages are exposed in python scripts?

mchitre commented 5 months ago

Yes, a warning when something fails would be good. Happy to accept a PR.

Arthanor commented 1 month ago

Hi, I don't know if it's because I have a wrong version (I have 1.8.4, from anaconda), but in my version, this has not been implemented, so the path error doesn't get reported and it just seems like bellhop failed.

I think that _models.append(('bellhop', _Bellhop)) on line 854 of uwapm.py, also makes it impossible for the typical pm.models() test to no list bellhop as well.

Combined, it makes it difficult for new users to identify path errors since, it seems like bellhop is there from the models list and, when trying to run it, the error is that it failed to generate files, not that it wasn't found. For such a common/basic error, it shouldn't be necessary to dig in and view internal function messages to know what is happening.