Bugs in 'mace.py', 'demo_mace4D.py', 'demo_mace4D_fast.py'

cabouman / mbircone

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

11 stars 9 forks source link

Bugs in 'mace.py', 'demo_mace4D.py', 'demo_mace4D_fast.py' #109

Open bommaritom opened 1 year ago

bommaritom commented 1 year ago

Hi Diyu,

I've noticed the following bugs in the master branch. They should be easy to fix.

mace.py, line 333: The deprecated argument ror_radius is used, for a later function call to recon on line 354.
- I propose to delete 'ror_radius':ror_radius, from line 333 of mace.py.
demo_mace4D.py, line 258: The argument save_path is used, but this argument does not exist in the function being called.
- I propose to delete this argument from the function call on line 258.
demo_mace4D_fast.py, line 270: The argument save_path is used, but this argument does not exist in the function being called.
- I propose to delete this argument from the function call on line 270.

I have implemented these three changes on a local branch, and currently waiting for the demo files to finish.

dyang37 commented 1 year ago

The fixes sound reasonable. Could you please push your local branch to upstream when you get a chance? I have not updated mace functions to align with recent interface changes, so I might need to add/modify things too.

bommaritom commented 1 year ago

Ok. Is there a branch currently in development that I can add these changes to? I don't want to clutter with too many branches given that this is a small change.

dyang37 commented 1 year ago

Ok. Is there a branch currently in development that I can add these changes to? I don't want to clutter with too many branches given that this is a small change.

I don't think there exist one. Perhaps you can create a branch called "mace_interface_update", and push your local changes to that? I'll take it from there tomorrow, and we can delete it after testing & merging into master, which should not take long.

bommaritom commented 1 year ago

Ok, it is named fix/mace_files.

dyang37 commented 1 year ago

I made some updates to the mace3D function yesterday. Let me go back and decouple the changes, so that we could merge in different components one piece at a time. I'll first submit a PR that only contains the interface & doctring changes of mace3D. Maybe @bommaritom could help me test that.

cabouman commented 1 year ago

Great idea! Do one small piece at a time and make sure it is done correctly.

bommaritom commented 1 year ago

Closing this because it was resolved in #112.

dyang37 commented 1 year ago

This is not completely done actually. I still need to modify the interface of mace4D.