Skip to content

bugfix to rebuild_platypus_population#276

Merged
quaquel merged 6 commits intomasterfrom
bugfix_optimization
Jun 14, 2023
Merged

bugfix to rebuild_platypus_population#276
quaquel merged 6 commits intomasterfrom
bugfix_optimization

Conversation

@quaquel
Copy link
Copy Markdown
Owner

@quaquel quaquel commented Jun 13, 2023

The current implementation of rebuild_platypus_population did not properly encode the decision variables. This means that a roundtrip via to_dataframe, rebuild_platypus_population, to_dataframe does not work for non-real parameters. This fixes that by properly encoding all decision variables.

decision variables are now properly encoded using the underlying platypus datatypes
@quaquel quaquel added the bug label Jun 13, 2023
@quaquel quaquel added this to the 2.5.0 milestone Jun 13, 2023
@quaquel quaquel requested a review from EwoutH June 13, 2023 09:03
@quaquel quaquel self-assigned this Jun 13, 2023
@EwoutH
Copy link
Copy Markdown
Collaborator

EwoutH commented Jun 13, 2023

Looks good. Two remarks:

  • Should we check somewhere that the length and order of problem.types and decision_variables match (and throw a useful error if it isn’t? Or will that always be the case?
  • Is there any test coverage on this code?

@quaquel
Copy link
Copy Markdown
Owner Author

quaquel commented Jun 13, 2023

  1. fair point. At a minimum, some assertion might be a good idea. It can only happen if you use a different model for to_problem than the one you used for the optimization.
  2. optimization.py is one of the parts of the workbench with the least code coverage. As part of the larger rework of the optimization, I want to address that as well. This is just a quick bug fix and not per se the place to address this coverage issue.

@quaquel
Copy link
Copy Markdown
Owner Author

quaquel commented Jun 14, 2023

I rechecked the code. There is no need for a check because problem.types and problem.parameter_names will always match. the bigger issue might be a mismatch between problem.parameter_names and what is in the row. This would entail a try-catch around the getattrr calls. The other error could be that there are more columns in the archive than the combined length of parameter_names and outcome_names. This can happen if the model used for to_problem does not match the model used to create the optimization results for which you are rebuilding the population. I'll add an explicit check for this as well.

@coveralls
Copy link
Copy Markdown

coveralls commented Jun 14, 2023

Coverage Status

coverage: 80.926% (-0.2%) from 81.097% when pulling ac6b326 on bugfix_optimization into cccad48 on master.

Copy link
Copy Markdown
Collaborator

@EwoutH EwoutH left a comment

Choose a reason for hiding this comment

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

Looks good, I added two suggestions to make the error messages more informative.

@quaquel quaquel merged commit 3b68895 into master Jun 14, 2023
@quaquel quaquel deleted the bugfix_optimization branch June 14, 2023 16:35
EwoutH added a commit that referenced this pull request Jun 18, 2023
* bugfix to rebuild_platypus_population

decision variables are now properly encoded using the underlying platypus datatypes

* some additional exceptions are now raised

* Update ema_workbench/em_framework/optimization.py

Co-authored-by: Ewout ter Hoeven <[email protected]>

* Update ema_workbench/em_framework/optimization.py

Co-authored-by: Ewout ter Hoeven <[email protected]>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: Ewout ter Hoeven <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants