search

cabouman / mbircone

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

Marco/dev #106

Closed bommaritom closed 1 year ago

bommaritom commented 1 year ago
gbuzzard commented 1 year ago

In the docstring for recon -> sharpness, this line was removed: Ignored if sigma_x is not None in qGGMRF mode, or if sigma_p is not None in proximal map mode. Why was this removed? It seems like important information.

Between recon and project, several of the inputs that are common between the two functions are listed in different orders. It would be better if they were listed in the same order for both functions, both in the signature and in the docstring. I would use the order from recon and change the order in project to match as much as possible.

Otherwise I think it looks ok.

bommaritom commented 1 year ago

Hi Prof. Buzzard,

I deleted that line because I thought it was superfluous with the descriptions of ‘sigma_x’ and ‘sigma_p’; on second thought it makes more sense to leave it in. I will add it back.

I’ll also make a commit where I reorder project and its docstrings to match the order of recon as much as possible.

I’ll create and push these two updates to the pull request shortly (after I send this email).

Gianmarco Bommarito @.***

From: gbuzzard @.> Reply-To: cabouman/mbircone @.> Date: Wednesday, December 21, 2022 at 4:47 PM To: cabouman/mbircone @.> Cc: Marco Bommarito @.>, Author @.***> Subject: Re: [cabouman/mbircone] Marco/dev (PR #106)

In the docstring for recon -> sharpness, this line was removed: Ignored if sigma_x is not None in qGGMRF mode, or if sigma_p is not None in proximal map mode. Why was this removed? It seems like important information.

Between recon and project, several of the inputs that are common between the two functions are listed in different orders. It would be better if they were listed in the same order for both functions, both in the signature and in the docstring. I would use the order from recon and change the order in project to match as much as possible.

Otherwise I think it looks ok.

— Reply to this email directly, view it on GitHubhttps://github.com/cabouman/mbircone/pull/106#issuecomment-1362135854, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AEU2ZH2NSY4YWWDBBMJU45TWON3H7ANCNFSM6AAAAAATD44RIM. You are receiving this because you authored the thread.Message ID: @.***>

gbuzzard commented 1 year ago

There were still a few arguments not in the same order, but I just fixed them and accepted the PR.