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

Inca motion vector (#376) #409

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

Conversation

aitaten
Copy link
Contributor

@aitaten aitaten commented Jul 23, 2024

Introduction of the correlation based motion vector computation as is currently computed in INCA. The error motion computation, and the NWP vector wind correction has not been included. However, this correlation based computation is similar to the original TREC computation adding a new OF method. The original idea is coming from #376

@aitaten aitaten added the enhancement New feature or request label Jul 23, 2024
@aitaten aitaten requested a review from dnerini July 23, 2024 15:13
@aitaten aitaten self-assigned this Jul 23, 2024
Copy link
Member

@dnerini dnerini left a comment

Choose a reason for hiding this comment

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

Thanks @aitaten this looks very cool! I left few initial comments to get started with the review

pysteps/motion/correlation.py Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

"correlation" sounds a bit too vague, can we use a more precise name? is this approach equivalent to "TREC"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TREC stands for tracking radar echo by cross-correlation (TREC). So I would say it is quite equivalent. I will change the name accordingly

pysteps/motion/correlation.py Outdated Show resolved Hide resolved

return IS_tmp, JS_tmp

else:
Copy link
Member

Choose a reason for hiding this comment

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

don't need this since the function won't be executed without numba

With *T* > 2, all the resulting obtained vectors are pooled together for
the final interpolation on a regular grid.

settings: dict, optional
Copy link
Member

Choose a reason for hiding this comment

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

where are these settings documented? can't we convert this dictionary into optional arguments?

Optional dictionary containing keyword arguments for the correlation
based algorithm.

interp_method: {"idwinterp2d", "rbfinterp2d"}, optional
Copy link
Member

Choose a reason for hiding this comment

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

out of curiosity, what approach uses INCA to densify the motion field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I does a cubic interpolation. I can also implemented if you want ... but again, It is done with numba :(

Copy link
Member

@dnerini dnerini Aug 5, 2024

Choose a reason for hiding this comment

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

do you think it could be added as an additional interpolator in the pysteps/utils/interpolate.py module? (I wouldn't mind the numba optional requirement)

if NUMBA_IMPORTED:

@jit(nopython=True)
def compute_motion_numba(image1, image2, NI, NJ, nthin, IS_tmp, JS_tmp):
Copy link
Member

Choose a reason for hiding this comment

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

can't we simply have a non accelerated version in case numba is not installed instead of throwing an error?

nr_fields = input_images.shape[0]
domain_size = (input_images.shape[1], input_images.shape[2])

PMIN = settings.get("PMIN", 0.05)
Copy link
Member

Choose a reason for hiding this comment

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

we usually use ALL_CAPS names for global constants only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks ... I did some changes on these and other variables

Copy link
Member

Choose a reason for hiding this comment

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

need to add numba to the test requirements

aitaten and others added 2 commits August 2, 2024 20:05
Co-authored-by: Daniele Nerini <daniele.nerini@gmail.com>
Co-authored-by: Daniele Nerini <daniele.nerini@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

2 participants