-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improving readability/consistency of errors/exceptions in gempyor
.
#313
Changes from 12 commits
452902f
9a7f4a3
c37a605
d063762
0cdeed1
add67d1
3b4e490
e3df5d8
2066ee4
2b3feed
72aaa84
4639721
064ec7e
f614127
9c20318
3b20e8a
afee285
2eaa427
48a78ee
6e1410f
5c09a3c
2785437
4cc651f
be85d72
fb549c4
135aa54
681ff79
1549b5b
cbfd093
ccfb6e0
e83f250
6eed1e3
08e25a3
e68dc8a
960f4d2
2b75d6a
5d999dd
a15184e
2b2a281
76b5adf
a0ae5e3
0c14d66
9a6ba69
4ca01af
88e310d
01d11a7
fd049dd
7096ef1
ef62ec3
99b4aec
d2f8cc2
cde6acb
7b4f6e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,7 +25,7 @@ def __init__(self, seir_config=None, compartments_config=None, compartments_file | |
self.times_set += 1 | ||
|
||
if self.times_set == 0: | ||
raise ValueError("Compartments object not set, no config or file provided") | ||
raise ValueError("Compartments object not set, no config or file provided.") | ||
return | ||
|
||
def constructFromConfig(self, seir_config, compartment_config): | ||
|
@@ -375,7 +375,9 @@ def get_comp_idx(self, comp_dict: dict, error_info: str = "no information") -> i | |
comp_idx = self.compartments[mask].index.values | ||
if len(comp_idx) != 1: | ||
raise ValueError( | ||
f"The provided dictionary does not allow to isolate a compartment: {comp_dict} isolate {self.compartments[mask]} from options {self.compartments}. The get_comp_idx function was called by'{error_info}'." | ||
f"The provided dictionary does not allow to isolate a compartment: {comp_dict} " | ||
f" isolate {self.compartments[mask]} from options {self.compartments}. " | ||
f"The get_comp_idx function was called by '{error_info}'." | ||
) | ||
return comp_idx[0] | ||
|
||
|
@@ -394,8 +396,7 @@ def get_transition_array(self): | |
if self.compartments["name"][compartment] == elem: | ||
rc = compartment | ||
if rc == -1: | ||
print(self.compartments) | ||
raise ValueError(f"Could not find {colname} defined by {elem} in compartments") | ||
raise ValueError(f"Could not find {colname} defined by {elem} in {self.compartments}.") | ||
emprzy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
transition_array[cit, it] = rc | ||
|
||
unique_strings = [] | ||
|
@@ -424,8 +425,11 @@ def get_transition_array(self): | |
candidate = reduce(lambda a, b: a + "*" + b, elem) | ||
candidate = candidate.replace(" ", "") | ||
# candidate = candidate.replace("*1", "") | ||
if not candidate in unique_strings: | ||
raise ValueError("Something went wrong") | ||
if candidate not in unique_strings: | ||
raise ValueError( | ||
f"Candidate '{candidate}' from 'rate' column is not in the list of unique strings. " | ||
f"Unique strings: {unique_strings}" | ||
emprzy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
rc = [it for it, x in enumerate(unique_strings) if x == candidate][0] | ||
transition_array[2][it] = rc | ||
|
||
|
@@ -464,8 +468,11 @@ def get_transition_array(self): | |
candidate = reduce(lambda a, b: a + "*" + b, y) | ||
candidate = candidate.replace(" ", "") | ||
# candidate = candidate.replace("*1", "") | ||
if not candidate in unique_strings: | ||
raise ValueError("Something went wrong") | ||
if candidate not in unique_strings: | ||
raise ValueError( | ||
f"Proportion exponent '{candidate}' is not found in the list of unique strings. " | ||
f"Unique strings: {unique_strings}" | ||
emprzy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
rc = [it for it, x in enumerate(unique_strings) if x == candidate][0] | ||
proportion_info[2][proportion_compartment_index] = rc | ||
proportion_compartment_index += 1 | ||
|
@@ -490,7 +497,10 @@ def get_transition_array(self): | |
if self.compartments["name"][compartment] == elem3: | ||
rc = compartment | ||
if rc == -1: | ||
raise ValueError(f"Could not find proportional_to {elem3} in compartments") | ||
raise ValueError( | ||
f"Could not find proportional_to {elem3} in compartments. " | ||
f"Available compartments: {self.compartments}" | ||
emprzy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
emprzy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
proportion_array[proportion_index] = rc | ||
proportion_index += 1 | ||
|
@@ -554,7 +564,10 @@ def parse_parameter_strings_to_numpy_arrays_v2(self, parameters, parameter_names | |
f = sp.sympify(formula, locals=symbolic_parameters_namespace) | ||
parsed_formulas.append(f) | ||
except Exception as e: | ||
print(f"Cannot parse formula: '{formula}' from parameters {parameter_names}") | ||
print( | ||
f"Cannot parse formula: '{formula}' :" | ||
f"from parameters: {parameter_names}." | ||
emprzy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
raise (e) # Print the error message for debugging | ||
|
||
# the list order needs to be right. | ||
|
@@ -600,10 +613,10 @@ def parse_parameter_strings_to_numpy_arrays( | |
not operators | ||
): # empty list means all have been tried. Usually there just remains one string in string_list at that time. | ||
raise ValueError( | ||
f"""Could not parse string {string_list}. | ||
This usually mean that '{string_list[0]}' is a parameter name that is not defined | ||
or that it contains an operator that is not in the list of supported operator: ^,*,/,+,-. | ||
The defined parameters are {parameter_names}.""" | ||
f"Could not parse string {string_list}. " | ||
f"This usually mean that '{string_list[0]}' is a parameter name that is not defined " | ||
emprzy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
f"or that it contains an operator that is not in the list of supported operators: ^,*,/,+,-. " | ||
emprzy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
f"The defined parameters are {parameter_names}." | ||
) | ||
|
||
split_strings = [x.split(operators[0]) for x in string_list] | ||
|
@@ -710,12 +723,14 @@ def list_access_element_safe(thing, idx, dimension=None, encapsulate_as_list=Fal | |
try: | ||
return list_access_element(thing, idx, dimension, encapsulate_as_list) | ||
except Exception as e: | ||
print(f"Error {e}:") | ||
print(f">>> in list_access_element_safe for {thing} at index {idx}") | ||
print(">>> This is often, but not always because the object above is a list (there are brackets around it).") | ||
print(">>> and in this case it is not broadcast, so if you want to it to be broadcasted, you need remove the brackets around it.") | ||
print(f"dimension: {dimension}") | ||
raise e | ||
raise Exception( | ||
f"Error {e}: " | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Saying "error" in an error message is redundant, and also unclear in this context. Maybe something like "Raised exception: e"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. n.b., there are some other case to apply this style to. |
||
f"in list_access_element_safe for {thing} at index {idx}. " | ||
f"This is often, but not always because the object above is a list (there are brackets around it). " | ||
f"and in this case it is not broadcast, so if you want to it to be broadcasted, you need remove the brackets around it. " | ||
f"dimension: {dimension}." | ||
) | ||
|
||
|
||
|
||
def list_access_element(thing, idx, dimension=None, encapsulate_as_list=False): | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -42,17 +42,17 @@ def validate_initial_file_check(cls, values): | |||||
initial_conditions_file = values.get('initial_conditions_file') | ||||||
initial_file_type = values.get('initial_file_type') | ||||||
if method in {'FromFile', 'SetInitialConditions'} and not initial_conditions_file: | ||||||
raise ValueError(f'Error in InitialConditions: An initial_conditions_file is required when method is {method}') | ||||||
raise ValueError(f"Error in InitialConditions: An initial_conditions_file is required when method is {method}") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Saying error in an exception is redundant. The output will list the fact that this is inside the |
||||||
if method in {'InitialConditionsFolderDraw','SetInitialConditionsFolderDraw'} and not initial_file_type: | ||||||
raise ValueError(f'Error in InitialConditions: initial_file_type is required when method is {method}') | ||||||
raise ValueError(f"Error in InitialConditions: initial_file_type is required when method is {method}") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
return values | ||||||
|
||||||
@model_validator(mode='before') | ||||||
def plugin_filecheck(cls, values): | ||||||
method = values.get('method') | ||||||
plugin_file_path = values.get('plugin_file_path') | ||||||
if method == 'plugin' and not plugin_file_path: | ||||||
raise ValueError('Error in InitialConditions: a plugin file path is required when method is plugin') | ||||||
raise ValueError(f"Error in InitialConditions: a plugin file path is required when method is plugin.") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
return values | ||||||
|
||||||
|
||||||
|
@@ -70,19 +70,31 @@ def validate_seedingfile(cls, values): | |||||
seeding_file_type = values.get('seeding_file_type') | ||||||
seeding_file = values.get('seeding_file') | ||||||
if method == 'PoissonDistributed' and not lambda_file: | ||||||
raise ValueError(f'Error in Seeding: A lambda_file is required when method is {method}') | ||||||
raise ValueError( | ||||||
f"Error in Seeding: A lambda_file is required when method is {method} " | ||||||
f"Current value: {lambda_file}." | ||||||
) | ||||||
if method == 'FolderDraw' and not seeding_file_type: | ||||||
raise ValueError('Error in Seeding: A seeding_file_type is required when method is FolderDraw') | ||||||
raise ValueError( | ||||||
f"Error in Seeding: A seeding_file_type is required when method is FolderDraw" | ||||||
f"Current value: {seeding_file_type}." | ||||||
) | ||||||
if method == 'FromFile' and not seeding_file: | ||||||
raise ValueError('Error in Seeding: A seeding_file is required when method is FromFile') | ||||||
raise ValueError( | ||||||
f"Error in Seeding: A seeding_file is required when method is FromFile " | ||||||
f"Current value: {seeding_file}." | ||||||
) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Like before, can drop the "error in location" portion of the exception message, python will take care of that for you. |
||||||
return values | ||||||
|
||||||
@model_validator(mode='before') | ||||||
def plugin_filecheck(cls, values): | ||||||
method = values.get('method') | ||||||
plugin_file_path = values.get('plugin_file_path') | ||||||
if method == 'plugin' and not plugin_file_path: | ||||||
raise ValueError('Error in Seeding: a plugin file path is required when method is plugin') | ||||||
raise ValueError( | ||||||
f"Error in Seeding: a plugin file path is required when method is plugin " | ||||||
f"Current value: {plugin_file_path!r}. Please specify the path to the plugin file." | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Drop the "error in location" and also there is an issue with the grammar. "Current" doesn't need to be capitalized and probably can do away with the colon. |
||||||
) | ||||||
return values | ||||||
|
||||||
class IntegrationConfig(BaseModel): | ||||||
|
@@ -107,11 +119,11 @@ def check_distr(cls, values): | |||||
b = values.get('b') | ||||||
if distr != 'fixed': | ||||||
if not mean and not sd: | ||||||
raise ValueError('Error in value: mean and sd must be provided for non-fixed distributions') | ||||||
raise ValueError(f"Mean and sd must be provided for non-fixed distributions.") | ||||||
if distr == 'truncnorm' and not a and not b: | ||||||
raise ValueError('Error in value: a and b must be provided for truncated normal distributions') | ||||||
raise ValueError(f"a and b must be provided for truncated normal distributions.") | ||||||
if distr == 'fixed' and not value: | ||||||
raise ValueError('Error in value: value must be provided for fixed distributions') | ||||||
raise ValueError(f"Value must be provided for fixed distributions") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
return values | ||||||
|
||||||
class BaseParameterConfig(BaseModel): | ||||||
|
@@ -130,7 +142,10 @@ def which_value(cls, values): | |||||
value = values.get('value') is not None | ||||||
timeseries = values.get('timeseries') is not None | ||||||
if value and timeseries: | ||||||
raise ValueError('Error in seir::parameters: your parameter is both a timeseries and a value, please choose one') | ||||||
raise ValueError( | ||||||
f"Configuration error in seir::parameters: your parameter is both a timeseries and a value, please choose one. " | ||||||
f"Current values - value: {values.get('value')!r}, timeseries: {values.get('timeseries')!r}." | ||||||
) | ||||||
return values | ||||||
|
||||||
|
||||||
|
@@ -203,7 +218,11 @@ def which_source(cls, values): | |||||
incidence = values.get('incidence') | ||||||
prevalence = values.get('prevalence') | ||||||
if incidence and prevalence: | ||||||
raise ValueError('Error in outcomes::source. Can only be incidence or prevalence, not both.') | ||||||
raise ValueError( | ||||||
emprzy marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
f"Configuration error in outcomes::source." | ||||||
f"Value can only be incidence or prevalence, not both." | ||||||
f"Current values - incidence: {values.get('incidence')!r}, prevalence: {values.get('prevalence')!r}." | ||||||
) | ||||||
return values | ||||||
|
||||||
# @model_validator(mode='before') # DOES NOT WORK | ||||||
|
@@ -256,7 +275,7 @@ def check_paramfromfile_type(cls, values): | |||||
param_subpop_file = values.get('param_subpop_file') is not None | ||||||
|
||||||
if param_from_file and not param_subpop_file: | ||||||
raise ValueError(f"Error in outcome: 'param_subpop_file' is required when 'param_from_file' is True") | ||||||
raise ValueError(f"Error in outcome: 'param_subpop_file' is required when 'param_from_file' is True.") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as before with the drop "error in location" and there's no need to use an f-string here. |
||||||
return values | ||||||
|
||||||
class ResampleConfig(BaseModel): | ||||||
|
@@ -320,9 +339,15 @@ def verify_inference(cls, values): | |||||
inference_present = values.get('inference') is not None | ||||||
start_date_groundtruth = values.get('start_date_groundtruth') is not None | ||||||
if inference_present and not start_date_groundtruth: | ||||||
raise ValueError('Inference mode is enabled but no groundtruth dates are provided') | ||||||
raise ValueError( | ||||||
f"Inference mode is enabled, but no groundtruth dates are provided." | ||||||
f"Please provide groundtruth dates." | ||||||
) | ||||||
elif start_date_groundtruth and not inference_present: | ||||||
raise ValueError('Groundtruth dates are provided but inference mode is not enabled') | ||||||
raise ValueError( | ||||||
f"Groundtruth dates are provided, but inference mode is not enabled." | ||||||
f"Please enable inference mode." | ||||||
) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for the f-strings and there's a missing space after the first period. |
||||||
return values | ||||||
|
||||||
@model_validator(mode='before') | ||||||
|
@@ -331,13 +356,17 @@ def check_dates(cls, values): | |||||
end_date = values.get('end_date') | ||||||
if start_date and end_date: | ||||||
if end_date <= start_date: | ||||||
raise ValueError('end_date must be greater than start_date') | ||||||
raise ValueError( | ||||||
f"`end_date` ({end_date}) must be later than `start_date` ({start_date}))." | ||||||
) | ||||||
return values | ||||||
|
||||||
@model_validator(mode='before') | ||||||
def init_or_seed(cls, values): | ||||||
init = values.get('initial_conditions') | ||||||
seed = values.get('seeding') | ||||||
if not init or seed: | ||||||
emprzy marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
raise ValueError('either initial_conditions or seeding must be provided') | ||||||
raise ValueError( | ||||||
f"At least one of `initial_conditions` or `seeding` must be provided." | ||||||
) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. f-string is not needed here and the indentation looks a bit off? |
||||||
return values |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -268,9 +268,9 @@ def autodetect_scenarios(config): | |
|
||
if len(seir_modifiers_scenarios) != 1 or len(outcome_modifiers_scenarios) != 1: | ||
raise ValueError( | ||
f"Inference only support configurations files with one scenario, got" | ||
f"seir: {seir_modifiers_scenarios}" | ||
f"outcomes: {outcome_modifiers_scenarios}" | ||
f"Inference only supports configuration files with one scenario, recieved:\n" | ||
f"SEIR modifiers: {seir_modifiers_scenarios}." | ||
f"Outcomes modifiers: {outcome_modifiers_scenarios}." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's get rid of the random new line and see if this can be worked into one to two sentences. |
||
) | ||
|
||
return seir_modifiers_scenarios[0], outcome_modifiers_scenarios[0] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there's a double space between the first and second line. I'm also not super familiar with the context here, but can this be edited for grammar as well. Something like:
"The provided dictionary does not allow an isolated compartment: {comp_dict}. It is isolated from the options {self.compartments}. Additional info: {error_info}."
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I happen to be looking at this particular exception again for reasons unrelated to this PR and I now think this should be a
LookupError
: https://docs.python.org/3/library/exceptions.html#LookupError. I think that better describes the issue at hand here.