aodn / python-aodntools

Repository for templates and code relating to generating standard NetCDF files for the Australia Ocean Data Network
GNU Lesser General Public License v3.0
10 stars 3 forks source link

Velocity aggregated time series product #92

Closed diodon closed 4 years ago

diodon commented 4 years ago

This product flattens UCUR, VCUR, WCUR and reference the values to its TIME and absolute DEPTH. The values are aggregated from all deployments at one site in an indexed ragged array structure with OBSERVATION and INSTRUMENT as the sole dimensions

ocehugo commented 4 years ago

@diodon, don't forget to fix the aggregate_timeseries part - tests are not passing and there is some warnings about duplicated fill_values in the ncwriter.

codecov[bot] commented 4 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@cc73b44). Click here to learn what that means. The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #92   +/-   ##
=========================================
  Coverage          ?   66.38%           
=========================================
  Files             ?        7           
  Lines             ?      946           
  Branches          ?      147           
=========================================
  Hits              ?      628           
  Misses            ?      291           
  Partials          ?       27
Impacted Files Coverage Δ
...eseries_products/velocity_aggregated_timeseries.py 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update cc73b44...d16d37b. Read the comment docs.

ocehugo commented 4 years ago

IMO: The most important structural changes were not applied :)

  1. The fact you need temporary files. b. Which is caused by a lack of earlier validation. c. Which also leads to the fact that, if there is anything wrong in the file, you will have dangling temporary files. d. Which will lead to cleanup callbacks/cases, calls to "rm", etc.
  2. The WCUR missing values.
  3. The way of global and custom options are set in the code (fixed options are not "globals", runtime options are not kwargs). a. Which may cause hard to test when code is changed, some new code to be incompatible or cases not being handle, etc.

Finally, I don't see the tests. For example, the HEIGHT above sensor raise issue could've been easily detected. Others:

  1. Test metadata
  2. Test DEPTH is correct/expected.
  3. No data where DEPTH is not defined.
  4. Data is valid and compares to original/unaggregated files. etc
mhidas commented 4 years ago

You applied my documentation suggestions to the wrong file (aggregated_timeseries.md instead of velocity_aggregated_timeseries.md)!

mhidas commented 4 years ago

Ok, I think this is good enough for a first go!