Change solution inform into a dataclass

[eirikurj]

Purpose

Realized I had this lying around locally from an earlier development. This PR replaces the standard dict with a dataclass that gets passed and used to print the optimizer exit code and the associated text. Functionally, there is should be no difference.
Additionally, slipped in one small update to the warning, printing the pyoptsparse versions that's in use and the one that the history was generated with. Can make this as separate PR if needed.

Expected time until merged

No rush

Type of change

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (non-backwards-compatible fix or feature)
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no API changes)
• Documentation update
• Maintenance update
• Other (please describe)

Testing

Checklist

• I have run flake8 and black to make sure the Python code adheres to PEP-8 and is consistently formatted
• I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
• I have run unit and regression tests which pass locally with my changes
• I have added new tests that prove my fix is effective or that my feature works
• I have added necessary documentation

[marcomangano]

See issue below:

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.19%. Comparing base (2b35f53) to head (76e77dd).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #437      +/-   ##
==========================================
+ Coverage   86.17%   86.19%   +0.02%     
==========================================
  Files          24       24              
  Lines        3406     3413       +7     
==========================================
+ Hits         2935     2942       +7     
  Misses        471      471              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ewu63]

Changes look OK. However, I have a slightly different implementation idea: what if we made SolutionInform an IntEnum which is created for each optimizer class, and the final outcome is an instance of this class which is the actual exit condition? I think that would lead to a slightly cleaner design. However, in the longer term I would actually prefer not storing this inform mapping ourselves since optimizers may change it up over time and we should not need to maintain it (esp when supporting multiple optimizer versions). So maybe it's not worth it.

[marcomangano]

I agree with @ewu63 minor comments but I think that other than that this PR could move forward. I see how the informs mapping could become a headache in the future - and probably some of them are already out of date, but changing that sounds like a bigger task which would involve extracting the exit messages from each optimizer source code.

I see how the IntEnum approach might make things a bit cleaner, but unless you have already a version of the code with that I don't think it is worth updating the current PR.

[marcomangano]

@ewu63 I think something went wrong with the rebasing, when I check the Files changed section I still see @eirikurj 's changes

[ewu63]

@ewu63 I think something went wrong with the rebasing, when I check the Files changed section I still see @eirikurj 's changes

I did not reimplement everything as an IntEnum, merely cleaned up his code a little bit + added a classmethod.

[marcomangano]

@ewu63 you beat me on time. I followed back the commits and saw you added the fromInforms method.

This LGTM, but just a couple of things:

• should we rename the function argument to sol_inform as you suggested?
• very minor, but bit confusing that the Solution class in pyOpt_solution takes:
  (self, optProb, xStar, fStar, lambdaStar, optInform, info):
  while when the class is instantiated within _createSolution in pyOpt_optimizer takes:
  (optProb, xStar, fStar, multipliers, sol_inform, info)
  Consistent naming (use multipliers, sol_inform) could help future debugging?

[ewu63]

I did rename solInform to sol_inform, but did not touch any class attributes e.g. optInform. I would like to, but this goes into bigger problems with backwards compatibility and PEP8 naming conventions so I'd like to punt that to the "near future" if possible. I'd rather do some refactoring and deduplicating code first. Would prefer to also sneak in #402 to the same patch release.

[marcomangano]

Sounds good. This and the unconstrained test PR are enough for a patch release. I will look at the other PR later today, we should merge that first since you already bumped the version here.

[kanekosh]

LGTM! Not merging this yet in case @eirikurj wants to check the changes @ewu63 made.

[eirikurj]

Sorry for the absence on this. @ewu63 thanks for updating the PR. Maybe I misunderstood your earlier comment, but was it your intent to change solInform to sol_inform in this PR (seems like some rebase issue happened)?
Here, the Solution class used optInform, so I was going for consistency. This variable, however, should not be an issue to change. I am also fine merging this as-is and refactor later, to avoid delaying further. Just let me know and I can make changes. I am fine with either.
I would however like to discuss the pep8 naming further, as this constantly comes up in our codes, and we should work based on some written guidelines. Perhaps we can discuss on this issue mdolab/.github#66 as I think we should try to enforce this.

[ewu63]

I guess there was some rebase issues, I renamed the variables but left everything else intact. I agree that we should discuss naming conventions in mdolab/.github#66. The PR is now ready.

[ewu63]

Uhh so technically this was no longer backwards compatible -- users can no longer access the previous fields, e.g. sol.optInform["value"] and instead must use sol.optInform.value. We can patch this but I wonder if we should yank the release..

[eirikurj]

Ah, yeah you are right, this is breaking backwards compatibility. I dont know how many are accessing this information from the sol.optInform, so hopefully the impact is minimal, but we should patch this to maintain the previous functionality.
If you think this is serious, we should keep this clean and by-the-book and mark this release broken on conda-forge and update the release notes for this particular release.