added units flag scatter

[daniel-caichac-DHI]

Added an automatic units recognition from dfs0.item for singlepoint compare object scatter plot.

• If not dfs0 is provided (or if user wants to override units) a new kword units=str can be specified, for instance if instead of automatic units detection units = ° user wanted to specify degrees north then flag units = ' °N' can be specified.
• Also, shortening of the very basic units was added ( metres -> m; second - >s; degrees -> °, and; meter per second -> m/s). For any other unit (eg cm/s) the automatic detection will just write 'centimetres per second' and it will be the user the one who should specify kword units = 'cm/s' . This could be improved in the future with a very long mapping dictionary of long unit name -> short unit name.
• Before this commit scatter plot looked like this
  

now looks like this with units on the side (no extra kword, units extracted automatically from dfs0)

• Still to do: Implement these changes in track-comparer object scatter plot (should be copy-pasting of the code)

[jsmariegaard]

Great initiative. But I think it's quite important that we ensure consistent units across all plot types. I don't think the units should be passed as an argument to each plot but rather set someplace central.

[daniel-caichac-DHI]

As of now the units are extracted from observation.py and if present (ie if data comes from dfs0 or dfsu) then they are automatically pasted in the plot.
The kword is there in case, I think from a pragmatic point of view, the user wants to manually override the units at the very end of the analysis (which is when plotting the results), as for instance if the plot is direction, by default now it will say MWD [degrees] and the user might just want to replace to MWD [°N-coming from] and for Wind WD [degrees] into WD [°N-Going to]. This will happen eventually and I think is easier to add the kword at the plot stage rather than going back to set it as a property as in observation._units="WD [°N-Going to]" which can also override the default x-label and y-label.

I understand what you mean of the central place, but this skill table I only implemented for the scatter plot and not for the timeseries or spatial skill (at least not yet). We already merged the table (without units) to the main branch, so at least I think that we should either step back from the last merge and remove the skill table, or, leave it as it is of now and then maybe think in another centralized way.

[daniel-caichac-DHI]

• I had to fix a problem with the units and units test file as only sec was being converted to s since it is eum.meter_per_sec but we were forgetting about wave periods which have eum.second units, which was giving me issues with the plotting titles (eg. Peak Wave Period [sond] as sec was being changed to s in the word second.
• Also I formatted the skill table to 2 decimals except for number of samples which is an integer, so now if skill_table=True then we get 2 decimals (if it is 9.4 m then we get 9.40 m), see example in SI in following figure
  

I am pushing a series of independent branches with small changes since I am making plots for production and need them to be self-explanatory. Maybe we could then move the skill_table somewhere else as jesper suggested but I feel like now it is in a good place, since when doing a Comparison one can ask for either comparer.skill to get a table or comparer.scatter to get a scatter plot, but scatter will not run the comparer.skill script inside if not explicitly requested with skill_table=True. By default is false as of now.

[daniel-caichac-DHI]

@ecomodeller pls don't forget this branch when you have some time!

[ecomodeller]

1 .The units flag doesn't work:

2. If I make a minimal change to make it work, units becomes inconsistent:

I think it is better to change the unit in a central place rather than in the call to scatter().

Units are also used in the timeseries plot.

[daniel-caichac-DHI]

Alright, I agree, let me try to think this through, in a central place we could also call the units for spatial_skill plots.

[daniel-caichac-DHI]

@ecomodeller I fully re-made how the units and skill table work.

• Units are now as original intended, an attribute in the observation class (either point or track) that is read from dfs0/dfsu or can be specified (overridden) by units='user units' kword, when constructing the observation object.
• These units are passed into the plots (as before, no change there), but now also into the skill_table.
• Skill table now works both in point comparison object and track comparison scatter plots, see examples below (distance of the skill table depends on the presence of colorbar and number of elements in colorbar binning)

Default plots (no units overwriting, read from file)

code:

plots:

Plots overwriting units

Same but for satellites, without and with units replacement
o1 = TrackObservation(dfs0_file, name='Sentinel-3a')
cc['Sentinel-3a'].scatter(figsize=(5,5))
cc['Sentinel-3a'].scatter(figsize=(5,5),skill_table=True)
cc['Sentinel-3a'].scatter(figsize=(5,5),show_hist=False,skill_table=True)
cc['Sentinel-3a'].scatter(figsize=(5,5),skill_table=['si','r2'])

same with units replacement, just 1 plot to exemplify
o1 = TrackObservation(dfs0_file, name='Sentinel-3a',units='inches/sec')
cc['Sentinel-3a'].scatter(figsize=(5,5),skill_table=True)

I think this is what everyone wanted

[daniel-caichac-DHI]

@ecomodeller I cannot check or close the requested changes for some reason

[daniel-caichac-DHI]

I think I addressed all these comments

[ecomodeller]

1. Install pytest test coverage plugin $pip install pytest-cov

2. Create test coverage html report $ pytest --cov=fmskill --cov-report=html

3. Find out which of the lines of codes you recently added that is not covered by a test
   Like these:
   

4. Add some tests to make sure it is possible to use the new options

5. Rerun test coverage

[daniel-caichac-DHI]

I am writing the tests now but I can see that despite my pytest running successfully and also some autotests being executed properly, it fails on the Full test / build (ubuntu-latest, 3.7) (pull_request) for some reason related to Xarray (nothing I have changed regarding that)

[daniel-caichac-DHI]

@ecomodeller I added a series of tests regarding the new functionality, also that made me modify the plotting a little bit to avoid some errors, and Jesper suggested to Temporarily remove python 3.7 tests as they are all failing due to some xarray update, we can add them later.

[jsmariegaard]

@ecomodeller I added a series of tests regarding the new functionality, also that made me modify the plotting a little bit to avoid some errors, and Jesper suggested to Temporarily remove python 3.7 tests as they are all failing due to some xarray update, we can add them later.

Or maybe stop supporting Python 3.7 !? - see this discussion in xarray: pydata/xarray#6138