[jonas-hurst]

fixes #362

• works when data.name is unset
• always returns an xr.DataArray (prev. a DataArray was returned when target_band was unset, and DataSet when it was set)
• when the original datacube has a secondary coordinate on the bands dimension, the NDVI band will have a default dummy value for this secondary coordinate

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.36%. Comparing base (1c4a4c2) to head (1d21a31).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #428      +/-   ##
==========================================
+ Coverage   83.15%   83.36%   +0.20%     
==========================================
  Files          39       39              
  Lines        2571     2597      +26     
==========================================
+ Hits         2138     2165      +27     
+ Misses        433      432       -1     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[pjonsson]

There are 4 consecutive assignments to a variable called "nd" (which I assume is short for normalized difference), and it seems unlikely that all 4 are the normalized difference.

If you rewrite this as:

thing = _add_missing_coords(nd.expand_dims(band_dim).assign_coords({band_dim: [target_band]}), data, band_dim)
nd = xr.concat([data, thing], dim=band_dim)

we can reduce that to 2 consecutive assignments, and the surrounding code still works.

(I don't know what "thing" should be called instead, but you probably know what it is and can name it properly since you're in the process of modifying this code.)

[jonas-hurst]

Disagree. Normalized difference is first computed, then dims and coords are changed to match the original datacube to which it will be added.
Also, your proposed change limits readability.

[jonas-hurst]

Thanks for your quick review. I addressed your comments in my latest commits!

[pjonsson]

The fill values are created based on the type, so I assume the concrete values are never used. Can't this function use np.empty() instead to avoid the cost of having to fill an entire array with dummy values?

[jonas-hurst]

the array is always of length 1. I therefore simplified this to np.array([fill])

[jonas-hurst]

I disagree with the use use of np.empty(). The arbitrary random data in the resulting array may be misinterpreted by users or downstream openEO processes

[pjonsson]

If it's an array of length 1 it doesn't take a lot of time to initialize the array.

I don't think the fill values are any less arbitrary, but I guess they're deterministic and that is a valuable trait in itself.

[clausmichele]

Thanks @jonas-hurst and @pjonsson for the contribution!

[przell]

@clausmichele, thanks for reviewing the pr. We would appreciate to use it. :)
Do you have any schedule on when you will release a new version containing this fix?

[clausmichele]

Thanks for the reminder @przell, I will do the release today.

[clausmichele]

@przell @jonas-hurst release done!