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

bugfix to rebuild_platypus_population #276

Merged
merged 6 commits into from Jun 14, 2023
Merged

bugfix to rebuild_platypus_population #276

merged 6 commits into from Jun 14, 2023

Conversation

quaquel
Copy link
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
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
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
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

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
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.

ema_workbench/em_framework/optimization.py Outdated Show resolved Hide resolved
ema_workbench/em_framework/optimization.py Outdated Show resolved Hide resolved
quaquel and others added 4 commits June 14, 2023 16:32
Co-authored-by: Ewout ter Hoeven <E.M.terHoeven@student.tudelft.nl>
Co-authored-by: Ewout ter Hoeven <E.M.terHoeven@student.tudelft.nl>
@quaquel quaquel merged commit 3b68895 into master Jun 14, 2023
16 of 20 checks passed
@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 <E.M.terHoeven@student.tudelft.nl>

* Update ema_workbench/em_framework/optimization.py

Co-authored-by: Ewout ter Hoeven <E.M.terHoeven@student.tudelft.nl>

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

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

---------

Co-authored-by: Ewout ter Hoeven <E.M.terHoeven@student.tudelft.nl>
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.

None yet

3 participants