Velocity aggregated time series product

[diodon]

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]

1. change files_to_agg to files or file_list or file_str_list -> there is no agg here and put the type intention.
2. Change file to filestr in the loop -> again type intention.
3. return [file for _,file in tuples] is clearer

[diodon]

changed

[ocehugo]

Wouldn't the allowed_dimensions and required_variables be better in a global variable (outside the functional scope?)?

[ocehugo]

I think using "nc.site_code" directly on the if is better - you are not adding any information or scope by naming it again.

[diodon]

ok

[ocehugo]

same as above

[diodon]

changed

[ocehugo]

nvalues means nothing here, hence intent is not clear.

e.g. get_number_of_vertical_cells is much better - I don't even need to read the docstring.

[ocehugo]

Actually,why not just return the HEIGHT and the TIME variables? The intent will be clearer in the function call - The name would be "get_dim_len" or alike.

[diodon]

I've renamed the function and variables for clarity. The objective is to return the number of values that result in the flattening of the grid

[ocehugo]

see comment below about this function - it's not necessary.

[ocehugo]

def flat_variable is clearer IMO

[diodon]

Agree, changed

[ocehugo]

You don't need to use the "nc" class as argument, you just need an array and two strings.
Hence, If it defined as def get_in_water(time, start_str, end_str),
the code call would be:

time = nc['TIME']
in_water_ind = get_in_water(time,nc['time_coverage_start'],nc['time_coverage_end'])

[ocehugo]

I just noticed that the variable name here is confusing - you use nc to reference to a xarray dataset. Just rename the variable to xrobj or xobj. I did the comment above expecting the arguments to be netCDF4 objects.

This is a case where type hints shines.

[diodon]

nc for xarray and ds to netcdf4 dataset

[mhidas]

Also, you can now just import this function from aggregated_timeseries (and eventually from the utils/common module)

[ocehugo]

"cut the entire dataset to"

[diodon]

added

[ocehugo]

This block of vars can be defined (promoted) to keyword arguments and be dynamically assigned, if required.

agg_options = {'varlist': ..., 'time_units': , ...] # or even a global dict
aggregate_velocity(...,varlist=[...],time_units=...). # or **agg_options

[diodon]

could be but we don't expect to aggregate anything else apart from those variables.

[ocehugo]

anyway, this is a good practice both codewise and intent wise. For example, when reading, why the function "aggregate velocity" does not provide the option to select which velocity/vars I want to aggregate?

Code wise, "default" options should hardly be defined in the scope of the function. Also, if you put as kwargs and need to change the parameters you just change the function call instead of changing the function code!

[ocehugo]

use random names - if calling more than one thread at the same time things can get ugly

[diodon]

changed to UUID name

[ocehugo]

sorted_files is better.

[diodon]

changed

[ocehugo]

in_water_only = in_water(nc)

[ocehugo]

or alike

[ocehugo]

dont hide the state mutation. In another words, do not reuse the name if the state changed - There is no copies here only views

[mhidas]

Are you sure it's only a view? The documentation (of Dataset.where used by in_water function) is not quite clear, but I reckon it returns a new Dataset object. This is quite a waste if you just want to count the number of data values.

Maybe it's not a big deal in terms of execution time, but in any case you could move the clipping into the if not error_list: clause so you're not applying it to files you then reject.

[ocehugo]

You actually dont need a 15 line get_nvalues function to execute two lines:

DIM_X = 1 if 'HEIGHT_ABOVE_SENSOR' not in nc else x['HEIGHT_ABOVE_SENSOR'].size
file_size = DIM_X*nc['TIME'].size

[ocehugo]

check first - move this line and the if statements atop

[ocehugo]

you can create a "good_files' list instead and get rid of the loop in line 175. You can print bad files or just estimate the bad files when reporting [x for x in file_agg if x not in good_files]

[ocehugo]

check emptiness instead of doing this.

[diodon]

zero is needed as the start index in the main loop.

[ocehugo]

After going down and back again I understand what this variable is.Hence, uou should rename it -> It;s not a varlen_list, it's an index list.

Also, you may pre-compute the start/end of the indexes (line 214/215) here instead and use it in the for loop (also easier to debug the index ranges).

[ocehugo]

create dictionary with options:

obs_float_template = {'datatype':'float','zlib':True,'dimensions':('OBSERVATIONS'),"fill_value":99999.0}
obs_byte_template = {'datatype:'byte','zlib':True,'dimensions':('OBSERVATIONS'),'fill_value':99}
...
vdefs['UCUR'] = {varname:'UCUR',**float_template}
vdefs['UCURqc'] = {varname:'UCURqc',**byte_template};
...
for vname,vopts in vdefs.items():
  vars[k] = ds.createVariable(**vopts)

[ocehugo]

There can be a case where WCUR is only found in some files...I would imagine that, if I asked for WCUR in varlist, I would have a partially filled vector in the case of missing variables in some files.

[ocehugo]

Again - it's not easy to guess what the the second argument returned by get_nvalues is because the function name is not useful (you have to dig into the docstring).

[diodon]

solved

[ocehugo]

The expression inside the parenthesis is a bit long. Since this is an assignement operation, I would do without a big parenthesis

TIME[start:end] = np.repeat(...)/one_day - epoch/one_day

The intent of normalization is much better (and should be slightly quicker).

[ocehugo]

I don't understand the use of this package - TStools. You are only getting netcdf attributes. do you really need to import this package to do that?

[diodon]

the package contains several utility functions. I use three of them in the code. In the future a refactored version of it will be a package with functions used by all product-generating scripts.

[ocehugo]

IMO it's not required -> it's just noise for very little gain

[mhidas]

get_nominal_depth returns the value of the NOMINAL_DEPTH variable, if it exists, and the global attribute instrument_nominal_depth otherwise, and this is used in all of the products, so it makes sense to have it in a common module.

[ocehugo]

another two options - should be kwargs.

[ocehugo]

call this block a function

set_global_attrs(ds,time_units,time_calendar,time_format,...
options={...}
set_global_attrs(ds,**options)

[ocehugo]

another case for a function output_name = rename_file(oldname,newname).
Also, you need to check for overwrites (rename to an already existing one).

Personally, I don't think you need to use "tmp" files or to rename at all - just do the kind of checks you are doing here and above before processing/concat the files (reading attributes, etc). If you think on this steps (check stuff, create a valid name) as a validation step, then you only spend time in valid inputs.

For example, What is the use of an entire concatenated array in memory if you cant find the site_code, facility_code, contributors, etc? Check the easy things first...

[ocehugo]

I imagine you probably have a lot of templates at the moment - maybe a time for refactoring?

For example, a lot of fields are probably repeats from other templates - maybe a template to rule them all, while small templates only put stuff to "overwrite" bits. This may also avoid spread typos, slightly different entries, and help in maintenance (imagine something need to change - you have to remember and change all templates...).

[diodon]

it is in the plan

[ocehugo]

@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.

[mhidas]

👍 Looks pretty good. Just a few more comments from me :)

[mhidas]

get_nominal_depth returns the value of the NOMINAL_DEPTH variable, if it exists, and the global attribute instrument_nominal_depth otherwise, and this is used in all of the products, so it makes sense to have it in a common module.

[mhidas]

Added a few comments on the documentation.

[mhidas]

Suggested change

This product provides aggregated U, V, and W velocity time-series files for each mooring site, without any interpolation or filtering, except for the exclusion of the out-of-water data. For the ADCP instruments, the absulte depth of the measuring cell is calculated using the `DEPTH` measured at the instrument and the `HEIGHT_ABOVE_SENSOR`, The Quality Control (QC) flags are preserved. All the (python) code used for the generation of the products is openly available on GitHub.

This product provides aggregated U, V, and W velocity time-series files for each mooring site, without any interpolation or filtering, except for the exclusion of the out-of-water data. For the profiling (ADCP) instruments, the absolute depth of the measuring cell is calculated using the `DEPTH` measured at the instrument and the `HEIGHT_ABOVE_SENSOR`, The Quality Control (QC) flags are preserved.

[diodon]

changed

[mhidas]

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

[mhidas]

@diodon @ggalibert Do you think this is clear enough?

[mhidas]

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