search

manoharan-lab / holopy

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

Lensdocs #357

Closed briandleahy closed 4 years ago

briandleahy commented 4 years ago

Fixes #345 .

@barkls I haven't compiled this yet into html. What is the best way for me to compile it so we can review it?

Thanks!

barkls commented 4 years ago

You can compile it locally with "make html" in the holopy/docs directory. You should also run "make doctest" or run the python test script in the holopy root directory. We can also configure readthedocs to build any branch (but not fork), so you might consider making a new branch on the manoharan-lab account for that purpose.

On Wed, 8 Jul 2020 at 16:19, Brian Leahy notifications@github.com wrote:

@briandleahy https://github.com/briandleahy requested your review on:

357 https://github.com/manoharan-lab/holopy/pull/357 Lensdocs.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/manoharan-lab/holopy/pull/357#event-3526872733, or unsubscribe https://github.com/notifications/unsubscribe-auth/AETUFJ6UWAZ5WYIQXBDZETDR2TIDPANCNFSM4OU5NYLA .

barkls commented 4 years ago

Travis failed on doctests since Lens isn't in the holopy.scattering.theory namespace (but it should be).

briandleahy commented 4 years ago

The failing test should be fixed by 8061306, although now there are other issues with argmin().....

barkls commented 4 years ago

I solved failing tests in #358 by replacing da.argmax() with np.argmax(da.data) as the deprecationwarning suggested.

As an aside, this change is for the better - I have been wishing xarray argmin/argmax operated the way they will soon!

briandleahy commented 4 years ago

I made some changes, moving most of the discussion in the calc tutorial to a "Which scattering theory should I choose?" class.

So this is viewable online, I pushed to manoharanlab also.

barkls commented 4 years ago

It looks like readthedocs won't pick this up until the branch is built. Specifically we need to commit something to this branch.

barkls commented 4 years ago

Live on readthedocs!

barkls commented 4 years ago

Thanks for doing this, it looks great! I did another code review - lots of comments, but nearly all of them are stupid formatting fixes based on how things appeared on readthedocs. Note that it's building from the manoharan-lab lensdocs branch but this PR is from the @briandleahy lensdocs branch. You can push to the manoharan-lab branch to hopefully get the docs to update, or make a new PR from there instead of this one.

barkls commented 4 years ago

Thanks for addressing all my comments! Everything looks good, except for the heading of the user guide section on scattering theories with lens. Travis is still failing but that should be fixed by #358. It looks like readthedocs is now doing a check here as well!

briandleahy commented 4 years ago

OK addressed the comments! Should I close and reopen the PR, so that it picks up the fixes in #358?

barkls commented 4 years ago

I merged it in. Keeping an eye on the Travis run on the develop branch after the merge so we can address any problems.

Solomon

On Tue, 28 Jul 2020 at 12:11, Brian Leahy notifications@github.com wrote:

OK addressed the comments! Should I close and reopen the PR, so that it picks up the fixes in #358 https://github.com/manoharan-lab/holopy/pull/358?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/manoharan-lab/holopy/pull/357#issuecomment-665132335, or unsubscribe https://github.com/notifications/unsubscribe-auth/AETUFJ3CRELMGE7JWLCIW23R532CHANCNFSM4OU5NYLA .