Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WIP Integration of Live WDD #72

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

WIP Integration of Live WDD #72

wants to merge 10 commits into from

Conversation

bangunarya
Copy link

@bangunarya bangunarya commented Mar 29, 2023 •
edited by sk1p
Loading

Contributor Checklist:

Current implementation

Integration of Live WDD algorithm to Ptychography40 repository.
The original code is given here : https://github.com/Ptychography-4-0/LiveWDD
List of the contribution.

  1. Notebook example with given dataset
  2. File for WDD

To Do List

  • Settle on an API (compare to SSB and CoMUDF APIs) - @uellue @sk1p
  • Create tests
  • Documentations
    • API docs
    • Example notebook(s)

@codecov
Copy link

codecov bot commented Mar 29, 2023 •
edited
Loading

Codecov Report

Attention: Patch coverage is 98.75776% with 4 lines in your changes missing coverage. Please review.

Project coverage is 85.08%. Comparing base (bdea089) to head (b7ecbbf).

Files with missing lines Patch % Lines
src/ptychography40/reconstruction/wdd/wdd_udf.py 97.82% 2 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
+ Coverage   77.18%   85.08%   +7.89%     
==========================================
  Files           8       12       +4     
  Lines         548      865     +317     
  Branches       84      130      +46     
==========================================
+ Hits          423      736     +313     
- Misses        110      112       +2     
- Partials       15       17       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sk1p
Copy link
Contributor

sk1p commented May 15, 2023

@bangunarya let me know if you need help with getting the tests green - I have an idea how to fix the issues.

@bangunarya
Copy link
Author

@bangunarya let me know if you need help with getting the tests green - I have an idea how to fix the issues.

Hi @sk1p, sure I will contact you on telegram

@sk1p
Copy link
Contributor

sk1p commented Jun 1, 2023

/azp run Ptychography-4-0.ptychography

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p
Copy link
Contributor

sk1p commented Jun 26, 2023

CI failure seems to come from ancient numba version used with Python 3.6 - it's probably time to drop that anyways. cc @uellue

@sk1p sk1p mentioned this pull request Jun 27, 2023
Closed
sk1p
sk1p reviewed Jun 27, 2023
View reviewed changes
Copy link
Contributor

@sk1p sk1p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is coming along nicely! It should even be possible to reach ~100% coverage for all added files. Mostly, the lines currently not covered are handling the different complex dtypes, and it would indeed be nice to ensure that in the future, it's still possible to pass in complex64.

The tests are now using complex128 because of precision issues, so there should be additional tests using the reduced precision (and one exercising the "invalid value" branch, making sure the correct error is raised). Because of the precision issue, allclose should have custom tolerances passed for these new tests.

There's also a branch for default transformation - that should also have a test case, making sure the code doesn't crash in case of not specifying the transformation.

Thank you for the patience with all these CI issues!

sk1p added a commit that referenced this pull request Jul 24, 2023
@sk1p
Infrastructure updates
dc0f294 
Extracted from #72
many thanks to @bangunarya for his contributions!

Co-authored-by: Arya Bangun <bangun@...>
@sk1p sk1p mentioned this pull request Jul 24, 2023
Merged
4 tasks
uellue pushed a commit that referenced this pull request Jul 24, 2023
@sk1p @uellue
Infrastructure updates
80d9624 
Extracted from #72
many thanks to @bangunarya for his contributions!

Co-authored-by: Arya Bangun <bangun@...>
@bangunarya
Copy link
Author

This is coming along nicely! It should even be possible to reach ~100% coverage for all added files. Mostly, the lines currently not covered are handling the different complex dtypes, and it would indeed be nice to ensure that in the future, it's still possible to pass in complex64.

The tests are now using complex128 because of precision issues, so there should be additional tests using the reduced precision (and one exercising the "invalid value" branch, making sure the correct error is raised). Because of the precision issue, allclose should have custom tolerances passed for these new tests.

There's also a branch for default transformation - that should also have a test case, making sure the code doesn't crash in case of not specifying the transformation.

Thank you for the patience with all these CI issues!

Thank you for the update of the infrastructure. Yes, I am thinking of adding more test, including some transformations.

@sk1p sk1p mentioned this pull request Aug 25, 2023
Open
sk1p
sk1p reviewed Aug 27, 2024
View reviewed changes
Copy link
Contributor

@sk1p sk1p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@uellue I've added some comments on your changes, hope they are helpful!

src/ptychography40/reconstruction/wdd/wdd_udf.py Outdated Show resolved Hide resolved
src/ptychography40/reconstruction/wdd/wdd_udf.py Outdated Show resolved Hide resolved
src/ptychography40/reconstruction/wdd/wdd_udf.py Outdated Show resolved Hide resolved
src/ptychography40/reconstruction/wdd/wdd_udf.py Show resolved Hide resolved
@sk1p

This comment was marked as outdated.

Sign in to view
Arya Bangun and others added 7 commits August 27, 2024 17:54
@uellue
Resolve pre-commit-config.yaml
91e839a
@uellue
Add wdd and test
82082c2
@uellue
- Add more coverages:
93fe422 
    - Rotation
    - different dtypes
- Fix linting
@uellue
Address failed test for windows dtype
d5d10db
@uellue
Coverages for pre_computed_Wiener
e7975c6
@uellue
Transformation in pre_computed_WIener
4bd924a
@uellue
Tiled live WDD
81a4087 
Reconstruct individual overlapping patches and merge the results in the end.

This should scale O(N) unless live plotting is enabled.
@uellue
Review @sk1p
05e503d 
Thx! :-)
@sk1p

This comment was marked as outdated.

Sign in to view
@sk1p

This comment was marked as outdated.

Sign in to view
@uellue
Faster stitching
b7ecbbf 
Add patches one by one instead of optimizing the whole set in `get_results()`. This required some additional
changes and tests for the stitching function.

Add the missing wdd.rst

Disable Numba for the larger test run. test_wdd_no_rot should cover the code for Numba, I think.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uellue uellue uellue left review comments

@sk1p sk1p sk1p left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bangunarya @sk1p @uellue