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.

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

A change in overlap setting. #334

Merged
merged 4 commits into from
Nov 19, 2024
Merged

A change in overlap setting. #334

merged 4 commits into from
Nov 19, 2024

Conversation

JUDEKEPA
Copy link
Contributor

@JUDEKEPA JUDEKEPA commented Nov 6, 2024

Main idea

The former overlap verification condition is that it is not allowed to larger or equal to window_size. It works well when window_size is equal to search_area_size, and any different spacing can be set for window centres (pixels of interest). But when search_area_size is larger than window_size, the largest overlap between each search window is the window_size - 1. That makes the window centres spacing cannot be smaller than search_area_size - window_size.

Here is an example:

  1. When the verification condition is overlap is not allowed to larger or equal to window_size.
    window_size = 32
    search_area_size = 35
    overlap = 33

    Output: ValueError("Overlap has to be smaller than the window_size")

The largest overlap can be set at 31 In this situation, the spacing between each window centre is 4.

  1. When the verification condition is overlap is not allowed to larger or equal to search_area_size.
    window_size = 32
    search_area_size = 35
    overlap = 34

This time, the spacing between each window centre is 0. Under this verification condition, the spacing is allowed to adjust with higher flexibility.

All tests has passed. I consider this change is safe. Because the function sliding_window_array also has nothing to to with window_size during its call in extended_search_area_piv.

Here is its usage:
# We implement the new vectorized code
aa = sliding_window_array(frame_a, search_area_size, overlap)
bb = sliding_window_array(frame_b, search_area_size, overlap)

Summary by Sourcery

Bug Fixes:

  • Fix the overlap verification condition to allow overlap to be smaller than the search_area_size instead of the window_size.

JUDEKEPA and others added 2 commits November 7, 2024 10:16
Copy link

sourcery-ai bot commented Nov 6, 2024

Reviewer's Guide by Sourcery

The PR modifies the overlap validation logic in the PIV (Particle Image Velocimetry) processing. Instead of checking if the overlap is smaller than the window_size, it now verifies that the overlap is smaller than the search_area_size. This change allows for more flexible window center spacing when the search area is larger than the window size.

No diagrams generated as the changes look simple and do not need a visual representation.

File-Level Changes

Change Details Files
Modified overlap validation condition in PIV processing
  • Changed overlap validation to compare against search_area_size instead of window_size
  • Updated error message to reflect new validation condition
  • Allows for smaller window center spacing when search_area_size > window_size
openpiv/pyprocess.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @JUDEKEPA - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@alexlib
Copy link
Member

alexlib commented Nov 15, 2024

Dear @JUDEKEPA - could you please explain in PIV terms why would we need an overlap larger than the window size? Even window size overlap means that we just duplicate the same particles over and over again, creating huge oversampling. Raffel's book recommends .25 to .5 overlap, and I agree this is the most recommended case. I'd like to see some cases where this large overlap is beneficial.

@JUDEKEPA
Copy link
Contributor Author

Dear @alexlib - actually I am working on using the Digital Image Correlation (DIC) method to observe some phenomenon in metal microstructure. And we need very large overlap to get a fine and dense displacement field. It is really useful in our work but I did not realise in PIV terms the large overlap is not necessary.

However, the other thing promotes me to change this setting is that, when I initially learned to use OpenPIV, I tried to use different values for window_size, search_area_size, and overlap. When I was using a large overlap I received the error message: 'ValueError("Overlap has to be smaller than the window_size")'. It is really easy for people to think that the overlap value is interacting with window_size instead of search_area_size. I tried to calculate the size of displacement field by hand based on the parameters, and I found the size of displacement field calculated rightly when the overlap value is interacting with search_area_size. I feel this change may be better for people to understand when they encounter with such error.

I am not an expert in PIV, so your opinion is more important whether the change should be made.

@alexlib
Copy link
Member

alexlib commented Nov 16, 2024

Dear @alexlib - actually I am working on using the Digital Image Correlation (DIC) method to observe some phenomenon in metal microstructure. And we need very large overlap to get a fine and dense displacement field. It is really useful in our work but I did not realise in PIV terms the large overlap is not necessary.

However, the other thing promotes me to change this setting is that, when I initially learned to use OpenPIV, I tried to use different values for window_size, search_area_size, and overlap. When I was using a large overlap I received the error message: 'ValueError("Overlap has to be smaller than the window_size")'. It is really easy for people to think that the overlap value is interacting with window_size instead of search_area_size. I tried to calculate the size of displacement field by hand based on the parameters, and I found the size of displacement field calculated rightly when the overlap value is interacting with search_area_size. I feel this change may be better for people to understand when they encounter with such error.

I am not an expert in PIV, so your opinion is more important whether the change should be made.

No problem that OpenPIV would also be useful for DIC. However, please create a Jupyter notebook with a test and some demo images that demonstrate this change's effect and influence. It wouldn't be very harmful if the backward compatibility is removed, but we do need to remind our users not to use unrealistic settings for PIV results. Some users get weird results using our software with the settings we never planned to use together and then "blame" the software for the results. Error message today is both software protection from failure and user protection from misuse of the PIV software :)

@JUDEKEPA
Copy link
Contributor Author

JUDEKEPA commented Nov 17, 2024

No problem that OpenPIV would also be useful for DIC. However, please create a Jupyter notebook with a test and some demo images that demonstrate this change's effect and influence. It wouldn't be very harmful if the backward compatibility is removed, but we do need to remind our users not to use unrealistic settings for PIV results. Some users get weird results using our software with the settings we never planned to use together and then "blame" the software for the results. Error message today is both software protection from failure and user protection from misuse of the PIV software :)

Totally agree. I made a showcase in jupyter notebook, the link is here. I add some explanation at the top of blocks.

@alexlib
Copy link
Member

alexlib commented Nov 18, 2024

Great @JUDEKEPA please move the notebook to the /openpiv/tutorials or /openpiv/tests/ where other notebooks are and send a message to the openpiv-users forum so people know about the change and its rationale. Thanks for the contribution.

@JUDEKEPA
Copy link
Contributor Author

@alexlib I have moved the notebook to the /openpiv/tutorials and sent a request to join the openpiv users google group. I will send a message once I have joined the group.

@alexlib
Copy link
Member

alexlib commented Nov 19, 2024

Great, thanks. Sorry I forgot to ask to complete the procedure by a) adding a unit test b) confirming all tests pass
@JUDEKEPA

@JUDEKEPA
Copy link
Contributor Author

@alexlib I have run the all tests under tests/ folder. All 35 items passed. I added one test item in test_process.py, and I chose three tricky values for overlap to ensure the dimension of u and v is normal. I attached report here. test_results.txt

Please check if everything is alright.

@alexlib alexlib merged commit 707437e into OpenPIV:master Nov 19, 2024
@alexlib
Copy link
Member

alexlib commented Nov 19, 2024

Great

it is unfortunately breaks the github action test, but it is probably not related to your pull request.

thanks for your contribution. please add more cool stuff

@JUDEKEPA
Copy link
Contributor Author

Thank you for your effort on OpenPIV. I'll try to contribute more. Please tell me if the change makes anything strange or unstable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants