cal-adapt / climakitae

A Python toolkit for retrieving, visualizing, and performing scientific analyses with data from the Cal-Adapt Analytics Engine.
https://climakitae.readthedocs.io
BSD 3-Clause "New" or "Revised" License
19 stars 2 forks source link

Raise useful error when wrong units are manually set #358

Closed nicolejkeeney closed 2 months ago

nicolejkeeney commented 2 months ago

Description of PR

Removed ability to retrieve selections manually from Select that have inconsistent data and units (i.e. retrieving 'inches' of 'Air Temperature at 2m')

I also wrote a few tests to confirm that an error is raised when you try to select bad units!

Type of change

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.

Try running the following, where you try to retrieve precipitation with an invalid unit that has been manually set

selections = ck.Select()
selections.variable = 'Precipitation (total)'
selections.units = "bad unit choice"
selections.retrieve()

The code should raise an error when you try to retrieve the data, and print out valid unit options for the variable of choice.

Units selected: bad unit choice
Valid units: mm, inches
ValueError: Selected unit is not valid for the selected variable.

If you can try some other variables and units that would be good!

Checklist:

elehmer commented 2 months ago

This looks good but two things I'm seeing:

1) I am seeing 2 errors returned, one from this code and then one from the retrieve failing. We should either find a way to just return the 1 error or change the text of the retrieve error to be even more general.

2) I think this should also apply to the read_catalog_from_csv function. So maybe put this code in a local function and then run it in both read_catalogs.

nicolejkeeney commented 2 months ago

This looks good but two things I'm seeing:

  1. I am seeing 2 errors returned, one from this code and then one from the retrieve failing. We should either find a way to just return the 1 error or change the text of the retrieve error to be even more general.
  2. I think this should also apply to the read_catalog_from_csv function. So maybe put this code in a local function and then run it in both read_catalogs.
  1. I will look into that!
  2. Maybe that would be better to do in a separate PR?
elehmer commented 2 months ago

This looks good but two things I'm seeing:

  1. I am seeing 2 errors returned, one from this code and then one from the retrieve failing. We should either find a way to just return the 1 error or change the text of the retrieve error to be even more general.
  2. I think this should also apply to the read_catalog_from_csv function. So maybe put this code in a local function and then run it in both read_catalogs.
  1. I will look into that!
  2. Maybe that would be better to do in a separate PR?

This looks good but two things I'm seeing:

  1. I am seeing 2 errors returned, one from this code and then one from the retrieve failing. We should either find a way to just return the 1 error or change the text of the retrieve error to be even more general.
  2. I think this should also apply to the read_catalog_from_csv function. So maybe put this code in a local function and then run it in both read_catalogs.
  1. I will look into that!
  2. Maybe that would be better to do in a separate PR?

This looks good but two things I'm seeing:

  1. I am seeing 2 errors returned, one from this code and then one from the retrieve failing. We should either find a way to just return the 1 error or change the text of the retrieve error to be even more general.
  2. I think this should also apply to the read_catalog_from_csv function. So maybe put this code in a local function and then run it in both read_catalogs.
  1. I will look into that!
  2. Maybe that would be better to do in a separate PR?

If you feel that you need to get the other in first, sure.

nicolejkeeney commented 2 months ago

I fixed both the points that @elehmer raised, and removed the try... except line in data_interface because it appears to be superfluous, as appropriate errors are raised earlier in the code pipeline (including for when too small of a region is attempted to be retrieved, multiple errors are called)