Adding a gradient algorithm by taobrienlbl · Pull Request #694 · LBL-EESA/TECA

taobrienlbl · 2022-05-17T15:57:51Z

No description provided.

burlen

Hi Travis, The calculation looks correct for a uniform Cartesian mesh. I found a few metadata related issues that should be fixed.

taobrienlbl · 2022-05-18T21:55:45Z

I believe I've addressed all your comments @burlen

@amandasd - do you have any?

amandasd · 2022-05-18T23:39:41Z

+    num_t *dest = grad_y;
+    num_t *src = grad_y + n_lon;
+    for (unsigned long i = 0; i < n_lon; ++i)
+        dest[i] = src[i+n_lon];


Wouldn't it be dest[i] = src[i] instead of dest[i] = src[i+n_lon] in line 130?

I see what you're saying. It does look like that, but I'm confused because this code reproduced a python version of the same boundary condition (implemented totally differently, so it's unlikely the same mistake was made twice).

@burlen, this is based on teca_vorticity.cxx (see line 117), so if there's a problem here, there is a problem there too. I always get turned around when thinking about pointer math though. What do you think?

I can confirm that this was a pointer math bug. (And it turns out there was a bug in the code I used to compare with the python-generated version, such that I wasn't actually comparing TECA's gradient at all.)

I've also submitted a fix to teca_vorticity.cxx.

@taobrienlbl sorry for late reply, agree. Good catch @amandasd!

amandasd · 2022-05-18T23:46:44Z

+            num_t dx = delta_x[j];
+
+            gx[0] = (f_x2[0] - f_x0[0]) / dx;
+            gy[0] = (f_y2[0] - f_y0[0]) / dx;


Wouldn't it be dy instead of dx in line 92?

yes, thank you on this and the one below!! The test wouldn't have caught this because dx=dy

this has been fixed, thanks!

amandasd · 2022-05-18T23:47:05Z

+            num_t dx = delta_x[j];
+
+            gx[0] = (f_x2[0] - f_x0[0]) / dx;
+            gy[0] = (f_y2[0] - f_y0[0]) / dx;


Wouldn't it be dy instead of dx in line 107?

this has been fixed, thanks!

amandasd · 2022-05-19T00:55:19Z

+    unsigned long n_lat, bool periodic_lon=true)
+{
+    size_t n_bytes = n_lat*sizeof(num_t);
+    num_t *delta_x = static_cast<num_t*>(malloc(n_bytes));


Isn't better to use std::vector<num_t> instead of explicitly malloc?

That would be more in the C++ style, yes. I copied the approach from teca_vorticity.cxx, so I'm wondering if there's a performance reason for using a dynamically allocated array rather than a vector: perhaps the overhead from bounds-checking in a vector? @burlen - do you have an opinion here on which approach should be used?

I ended up going with std::vector since the [] operator doesn't have any overheads.

@taobrienlbl malloc is preferable. std::vector<T> makes an extra pass over the array initializing all elements to T(). For POD types this zeros the values out. This is wasted cycles because you are immediately initializing after. There's also substantially more overhead when resizing the std::vector vs. using realloc, since the std::vector will always allocate a new buffer and copy the contents where as realloc can extend the memory region in place. The latter doesn't impact you here.

OK, I'll revert it.

@taobrienlbl It's fine to use vector here, I didn't mean for you to have to revert this, just wanted to give an answer as to why I use malloc.

taobrienlbl · 2022-05-19T22:56:17Z

Oh I see, thanks for clarifying. It’s been reverted in any case.

…

On May 19, 2022 at 6:52:43 PM, Burlen Loring ***@***.******@***.***)) wrote: @burlen commented on this pull request. In alg/teca_gradient.cxx(https://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FLBL-EESA%2FTECA%2Fpull%2F694%23discussion_r877598893&data=05%7C01%7Cobrienta%40iu.edu%7C60852b51e822452f491b08da39ea548c%7C1113be34aed14d00ab4bcdd02510be91%7C0%7C0%7C637885975852161402%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=F%2F72ertxgay3cie4bNajlxrihArQENG1X5ssCyBuFek%3D&reserved=0): > +template <typename num_t> +constexpr num_t deg_to_rad() { return num_t(M_PI)/num_t(180); } + +template <typename num_t> +constexpr num_t earth_radius() { return num_t(6371.0e3); } + +// compute the gradient using second-order, centered finite difference +// assumes fixed mesh spacing. here we add periodic bc in lon +// and apply unit stride vector optimization strategy to loops +template <typename num_t, typename pt_t> +void gradient(num_t *grad_x, num_t *grad_y, const pt_t *lon, const pt_t *lat, + const num_t *f, unsigned long n_lon, + unsigned long n_lat, bool periodic_lon=true) +{ + size_t n_bytes = n_lat*sizeof(num_t); + num_t *delta_x = static_cast<num_t*>(malloc(n_bytes)); @taobrienlbl(https://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftaobrienlbl&data=05%7C01%7Cobrienta%40iu.edu%7C60852b51e822452f491b08da39ea548c%7C1113be34aed14d00ab4bcdd02510be91%7C0%7C0%7C637885975852161402%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=BvZaVRWZgHvIr%2F0eGvGmVnBfPW4aMjkaHqLltwIkSYs%3D&reserved=0) It's fine to use vector here, I didn't mean for you to have to revert this, just wanted to give an answer as to why I use malloc. — Reply to this email directly, view it on GitHub(https://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FLBL-EESA%2FTECA%2Fpull%2F694%23discussion_r877598893&data=05%7C01%7Cobrienta%40iu.edu%7C60852b51e822452f491b08da39ea548c%7C1113be34aed14d00ab4bcdd02510be91%7C0%7C0%7C637885975852161402%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=F%2F72ertxgay3cie4bNajlxrihArQENG1X5ssCyBuFek%3D&reserved=0), or unsubscribe(https://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FACDDUFVSUZU2AZYT5TRPYZDVK3A3XANCNFSM5WFNTYFQ&data=05%7C01%7Cobrienta%40iu.edu%7C60852b51e822452f491b08da39ea548c%7C1113be34aed14d00ab4bcdd02510be91%7C0%7C0%7C637885975852161402%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=%2F2NDtnaIcfDDAgNNZ09RxGMrR%2B6CJIPWfJ2xyFfje6I%3D&reserved=0). You are receiving this because you were mentioned.Message ID: ***@***.***>

taobrienlbl · 2022-05-20T00:10:09Z

@burlen - when and if you think this is ready, would you please approve for merging?

taobrienlbl force-pushed the teca_gradient branch from d3f825e to 9d8c481 Compare May 17, 2022 23:48

taobrienlbl requested review from amandasd and burlen May 17, 2022 23:49

taobrienlbl changed the title ~~A new algorithm for calculating the gradient of a scalar field~~ Adding a gradient algorithm May 17, 2022

taobrienlbl force-pushed the teca_gradient branch from 9d8c481 to f2383d9 Compare May 18, 2022 16:15

burlen reviewed May 18, 2022

View reviewed changes

Comment thread alg/teca_gradient.cxx

Comment thread alg/teca_gradient.cxx Outdated

Comment thread alg/teca_gradient.cxx Outdated

Comment thread alg/teca_gradient.cxx

taobrienlbl force-pushed the teca_gradient branch from f2383d9 to 31d3ca4 Compare May 18, 2022 21:03

amandasd reviewed May 18, 2022

View reviewed changes

amandasd reviewed May 19, 2022

View reviewed changes

taobrienlbl force-pushed the teca_gradient branch 2 times, most recently from 5fc44f9 to c82afe3 Compare May 19, 2022 19:20

taobrienlbl force-pushed the teca_gradient branch from c82afe3 to d97e9f8 Compare May 20, 2022 12:18

taobrienlbl added 11 commits May 24, 2022 15:20

Wrote teca_gradient.cxx

64e937d

Added test for teca_gradient.cxx

554db81

Fixed spelling error/bug in apps/teca_bayesian_ar_detect.cpp

7905e1a

Fix to polar boundary condition issue in teca_vorticity.cxx

79c47cf

Added CF writer metadata for teca_evaluate_expression

c850d6d

Fixed variable name and documentation in teca_laplacian

bebc37f

squash: add CF writer attributes to laplacian

3421be6

fixes to allow -DTECA_DEBUG globally

0f8f41e

Allow high pass for constant weights in time_axis_convolution

a03f2e3

Change units of gradient and laplacian from m to km

760ab89

Allow teca_binary_segmentation to deal with NaNs

9d54859

taobrienlbl force-pushed the teca_gradient branch from d1d4650 to 9d54859 Compare May 27, 2022 21:16

Added intersection over union algorithm

37b85a1

taobrienlbl added 2 commits June 10, 2022 17:50

squashme: added test and fixed compilation for teca_iou.cxx

09c775f

squash: fixed IOU tests

0e0bfa5

Uh oh!

Conversation

taobrienlbl commented May 17, 2022

Uh oh!

burlen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

taobrienlbl commented May 18, 2022

Uh oh!

amandasd May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amandasd May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

burlen May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taobrienlbl commented May 19, 2022 via email

Uh oh!

taobrienlbl commented May 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

amandasd May 18, 2022 •

edited

Loading

amandasd May 18, 2022 •

edited

Loading

burlen May 19, 2022 •

edited

Loading