Bugfix/mace arg parse - Githubissues

cabouman / mbircone

BSD 3-Clause "New" or "Revised" License

11 stars 9 forks source link

Bugfix/mace arg parse #37

Closed dyang37 closed 3 years ago

dyang37 commented 3 years ago

Bug fix for incorrect denoiser_args unpacking in mace3D. For more details please see #36 . Tested demo_mace_3D.py and demo_mace_3D_fast.py, and both demo work. Also tested with denoiser function that accepts two additional arguments (i.e. denoiser_args contains two elements), which is the real reason why we implement this bug fix. The function and test script is not in this branch. To test this yourself please checkout experiments/metal_weld_mace3D.py and demo/denoiser_utils.py from branch lilly_experiments.

DamonLee5 commented 3 years ago

According to Greg's email,

We should fix the below 4 bugs before merging.

I know people think I'm anal for saying it's important to step through every line of code, but there is a bug in the very first line of clean_install_with_docs.sh! There were 5 of us on the call when we did this, so we should have caught it, but we didn't step through the code, and we didn't put in the symbols that would terminate the script when a line failed, so the error message must have just scrolled on by. (Nevertheless, I really like this script!)
There is an error in the docstring for mbircone.cone3D.auto_sigma_prior. There should be an extra return before Args.
There is an error in the docstring for mbircone.mace.denoiser_wrapper. The * should be preceded by a backslash.
There is another warning in building the docs: mbircone/docs/source/preprocess.rst:10: WARNING: autosummary: failed to import preprocess

dyang37 commented 3 years ago

I think all four issues above are fixed:

Bug in the first line of clean_install_with_docs.sh
Error in the docstring for mbircone.cone3D.auto_sigma_prior. There should be an extra return before Args.
Error in the docstring for mbircone.mace.denoiser_wrapper. The * should be preceded by a backslash. (I fixed this in a slight different way, in order to make the docstring consistent with the docstring of mace.mace3D. It is also the way how scipy handles the docstring formatting of a callable function object.)
Warning in building the docs: mbircone/docs/source/preprocess.rst:10: WARNING: autosummary: failed to import preprocess

For the test script experiments/metal_weld_mace3D.py, it might be hard for you to test it since it is a script that uses dask multi-node on Purdue cluster. It works on my end, and maybe @DamonLee5 can help me test it on his end as well.

This PR is ready for merge I think.

DamonLee5 commented 3 years ago

Tested it. The demo works and the docs building looks correct to me.

cabouman commented 3 years ago

OK, I'll accept it. We can continue testing it, and fix any additional bugs as they come up.