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

Use PlatypusConfig.default_variator #369

Merged
merged 2 commits into from
Sep 9, 2024

Conversation

dhadka
Copy link
Contributor

@dhadka dhadka commented Sep 8, 2024

Version 1.3.0 moved default_variator from a standalone method to part of PlatypusConfig. Thus, this updates the method reference and puts a minimum version on the dependency.

Alternatively, if you want to allow either version, you could use something like:

def default_variator(problem):
    try:
        return PlatypusConfig.default_variator(problem)
    except AttributeError:
        from platypus import default_variator
        return default_variator(problem)

@EwoutH
Copy link
Collaborator

EwoutH commented Sep 8, 2024

Thanks a lot for not only getting back, but also directly suggesting/implementing two potential fixes!

@quaquel, for your context, this PR originated from an issue I filed to Platypus after seeing our weekly CI fail: Project-Platypus/Platypus#246

@quaquel
Copy link
Owner

quaquel commented Sep 8, 2024

Thanks @dhadka! I'll take a closer look as soon as possible.

@dhadka
Copy link
Contributor Author

dhadka commented Sep 9, 2024

FYI, I've decided to put those methods back since it's a breaking change, but probably with a warning to switch to the new methods. I should have the version out in the next few hours. Though I would still recommend switching to the class version eventually.

@EwoutH
Copy link
Collaborator

EwoutH commented Sep 9, 2024

Thanks! The diligent follow ups are really appreciated!

@quaquel we have two options:

  1. Accept this PR and set Platypus 1.3 (or 1.3.1) as minimum version
  2. Use the other suggestion with the try-except block

@quaquel
Copy link
Owner

quaquel commented Sep 9, 2024

I am fine with option 1, requires updating the toml I guess?

@EwoutH
Copy link
Collaborator

EwoutH commented Sep 9, 2024

Yes, and this PR also already does that!

Thanks @dhadka, really appreciated.

@quaquel we probably want to release a bugfix release. Anything else that should be in there?

@EwoutH EwoutH merged commit 59cad47 into quaquel:master Sep 9, 2024
8 of 10 checks passed
@quaquel
Copy link
Owner

quaquel commented Sep 10, 2024

@quaquel we probably want to release a bugfix release. Anything else that should be in there?

checking all closed PRs since 2.5 in December, nothing seems API breaking and #307 which is massively breaking 2.x is not merged yet, so I think we can just include all PRs since the last release.

@EwoutH EwoutH added the bug label Sep 11, 2024
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