cabouman / mbircone

BSD 3-Clause "New" or "Revised" License
11 stars 9 forks source link

Reorganize #33

Closed dyang37 closed 3 years ago

dyang37 commented 3 years ago

Reorganized code structure as follows:

Screen Shot 2021-10-21 at 11 14 54 AM

Updated docs, setup.py, init.py, .gitignore files accordingly.

Tested demo_3D_shepp_logan.py and demo_mace3D_fast.py to make sure nothing is broken.

cabouman commented 3 years ago

This looks fine to me. Wenrui, can you check this? If you think it is OK, then I'll accept the PR.

DamonLee5 commented 3 years ago

I tested this branch on the demo/examples/metal_weld_3D.py to test the preprocess module, which gave me an error like this.

(mbircone_mace) li3120@brown-a229:/depot/bouman/users/li3120/mbircone/demo/examples $ python metal_weld_3D.py 
Traceback (most recent call last):
  File "metal_weld_3D.py", line 18, in <module>
    NSI_system_params = mbircone.preprocess.read_NSI_params(
AttributeError: 'function' object has no attribute 'read_NSI_params'
DamonLee5 commented 3 years ago

This error is because I used the same name "preprocess" for both the module and a function inside. Fixed it by renaming in 81adb96

cabouman commented 3 years ago

OK, should a accept the pull PR?

gbuzzard commented 3 years ago

Is it really appropriate to import all of the functions this way in the init.py file:
from .preprocess import from .cone3D import from .mace import mace3D from .phantom import *

For instance, cone3D has a number of functions that start with "_" and appear to be internal functions rather than functions to export as part of the mbircone module.

dyang37 commented 3 years ago

Is it really appropriate to import all of the functions this way in the init.py file: from .preprocess import from .cone3D import from .mace import mace3D from .phantom import *

For instance, cone3D has a number of functions that start with "_" and appear to be internal functions rather than functions to export as part of the mbircone module.

That's a good point. However I think this issue is probably more related to the issue of what function docs we should include in the sphinx docs.

BTW this issue also applies to svmbir. Currently svmbir has the same way of importing everything:

Screen Shot 2021-10-22 at 10 04 37 AM

I would suggest that given everything is tested and working for this pull request, we merge it to the master. We can later have a more detailed discussion regarding what functions we should give user access to.

cabouman commented 3 years ago

OK, given that this is working, I'm going to accept the PR, but let's discuss what functions are in-and-out of the documentation after the code review.