search

manoharan-lab / holopy

Hologram processing and light scattering in python
GNU General Public License v3.0
133 stars 50 forks source link

Rename FitResult properties #306

Closed barkls closed 4 years ago

barkls commented 4 years ago

Fixes #266 A couple notes:

I really struggled with what to call the result of a forward calculation using MAP parameter values from a fitting/MCMC calculation. Previously this was called best_fit; the best I could come up with (after brainstorming with @carolinesmartin) now is inferred_hologram. This name isn't great because HoloPy supports doing inference with arbitrary forward functions, so then the result of your inference on another scattering calculation would be called a "hologram" which is confusing/wrong. If anyone has a suggestion for a better name, please let me know!

This PR is against the branch in #305 since it builds on those changes. It can be merged before or after that PR, depending on how much revision they each require.

barkls commented 4 years ago

Thanks for the review! I fixed both issues. I think it's safe to merge this and #305 in either order, so I defer to you on that.

vnmanoharan commented 4 years ago

Minor point: do we really need the inferred_ prefixes? I understand that you want to distinguish from the guess_ values but since the object is called FitResults, it would probably be clear that, for example, FitResults.parameters refers to the results and not the guess.

barkls commented 4 years ago

I agree with @vnmanoharan. What do you think @briandleahy?

On Wed., Nov. 20, 2019, 06:17 vnmanoharan, notifications@github.com wrote:

Minor point: do we really need the inferred prefixes? I understand that you want to distinguish from the guess values but since the object is called FitResults, it would probably be clear that, for example, FitResults.parameters refers to the results and not the guess.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/manoharan-lab/holopy/pull/306?email_source=notifications&email_token=AETUFJ6IP4PCD3TDKGEQRPDQUUMDJA5CNFSM4JOAITQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEERUGTQ#issuecomment-555959118, or unsubscribe https://github.com/notifications/unsubscribe-auth/AETUFJ3KGBB4R2WF64P5HU3QUUMDJANCNFSM4JOAITQA .

briandleahy commented 4 years ago

I agree with @vnmanoharan. What do you think @briandleahy?

I am fine with the current version since right now FitResults also knows about the initial parameters, but I defer to you two. I think clearer is always better, but clearer is in the eye of the beholder and redundant and clear are not the same thing. So I defer to you two.

vnmanoharan commented 4 years ago

In that case, let's remove the inferred_ prefixes, since they are redundant.

barkls commented 4 years ago

Not sure why travis hasn't been triggered. Closing and reopening to see if that helps.

barkls commented 4 years ago

Travis still isn't showing the completed build. However, you can see that it passes by clicking on the "Details" link above.

Ready to merge @briandleahy?

briandleahy commented 4 years ago

Yeah I don't know what's going on. I'm going to merge it; we can always un-do / fix it if there is a problem.