Skip to content
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

Casting bound values as float in inference_parameter.py to prevent parsing errors #405

Merged
merged 4 commits into from
Dec 6, 2024

Conversation

emprzy
Copy link
Collaborator

@emprzy emprzy commented Nov 22, 2024

Describe your changes.

Casting lb and ub values as floats within the .add_single_parameter() method.

Does this pull request make any user interface changes? If so please describe.

N/A

What does your pull request address? Tag relevant issues.

This pull request addresses GH #380. Instances of scientific notation in configs that don't have a decimal in the mantissa AND a sign in the exponent are read in as strings (as opposed to floats), and this circumvents that.

Tag relevant team members.

@saraloo , is this adequate enough to prevent parsing errors? Perhaps there is somewhere else in this file (or places in other files) that needs a float() cast as well?

Cast `lb` and `ub` as `floats` when they are pulled from the config.
@TimothyWillard TimothyWillard added bug Defects or errors in the code. gempyor Concerns the Python core. config Relating to configuration files or their framework. low priority Low priority. quick issue Short or easy fix. labels Nov 25, 2024
@emprzy emprzy requested a review from saraloo December 4, 2024 17:41
@@ -53,8 +53,8 @@ def add_modifier(self, pname, ptype, parameter_config, subpops):
pname=pname,
subpop=sp,
pdist=parameter_config["value"].as_random_distribution(),
lb=parameter_config["value"]["a"].get(),
ub=parameter_config["value"]["b"].get(),
lb=float(parameter_config["value"]["a"].get()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason this is of a different form to lines 68 and 69? Otherwise I think this will deal with the error. thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops nope, just fixed them so that they match. Good catch

@emprzy emprzy marked this pull request as ready for review December 6, 2024 14:49
@TimothyWillard
Copy link
Contributor

Two quick things:

  1. Let's wait to merge this until after next Wed (12/11), folks are already testing the release candidate so this would be better for the next round, and
  2. I think a quick unit test would be super helpful, can be very small but something that emulates the problem described in [Bug]: Parsing error in modifiers/inference parameter (checking in bounds?) #380 and fails before the change but passes after this fix.

@emprzy emprzy merged commit b32cad8 into HopkinsIDD:dev Dec 6, 2024
1 check passed
@emprzy emprzy deleted the scientific_notation_parsing branch December 9, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Defects or errors in the code. config Relating to configuration files or their framework. gempyor Concerns the Python core. low priority Low priority. quick issue Short or easy fix.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Parsing error in modifiers/inference parameter (checking in bounds?)
4 participants