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

Fix SNOPT printout encoding and skip printing import warnings #392

Merged
merged 7 commits into from
Apr 24, 2024

Conversation

ewu63
Copy link
Collaborator

@ewu63 ewu63 commented Apr 18, 2024

Purpose

Closes #379.
Turns out we just forgot to initialize the cw array to " ". Also can get rid of cdummy now.

Also cleaned up import warnings for compiled libraries -- they are now no longer printed by default at import time for non-default libraries.

Expected time until merged

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

@ewu63 ewu63 requested a review from a team as a code owner April 18, 2024 20:56
@ewu63 ewu63 requested review from lamkina and sseraj April 18, 2024 20:56
Copy link

codecov bot commented Apr 18, 2024

Codecov Report

Attention: Patch coverage is 7.69231% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 61.66%. Comparing base (ab1618f) to head (73848dd).

Files Patch % Lines
pyoptsparse/pySNOPT/pySNOPT.py 0.00% 4 Missing ⚠️
pyoptsparse/pyOpt_utils.py 33.33% 2 Missing ⚠️
pyoptsparse/pyCONMIN/pyCONMIN.py 0.00% 1 Missing ⚠️
pyoptsparse/pyNSGA2/pyNSGA2.py 0.00% 1 Missing ⚠️
pyoptsparse/pyOpt_constraint.py 0.00% 1 Missing ⚠️
pyoptsparse/pyOpt_gradient.py 0.00% 1 Missing ⚠️
pyoptsparse/pyOpt_optimization.py 0.00% 1 Missing ⚠️
pyoptsparse/pySLSQP/pySLSQP.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #392       +/-   ##
===========================================
- Coverage   73.75%   61.66%   -12.10%     
===========================================
  Files          22       22               
  Lines        3296     3292        -4     
===========================================
- Hits         2431     2030      -401     
- Misses        865     1262      +397     

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

@ewu63
Copy link
Collaborator Author

ewu63 commented Apr 22, 2024

bump @sseraj @lamkina

Copy link
Contributor

@marcomangano marcomangano left a comment

Choose a reason for hiding this comment

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

Can you update the PR title/description to mention the new flag? I am also not sure if this is the way to go, see below.
The cw update is good stuff!

@@ -600,7 +605,7 @@ def try_import_compiled_module_from_path(module_name: str, path: Optional[str] =
try:
module = importlib.import_module(module_name)
except ImportError as e:
if path is not None:
if raise_warning:
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the scenarios where we don't want to raise a warning when the import fails? Even if it's from THIS_DIR we should probably raise an error

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right now on the main branch, running e.g. example/hs015.py will emit the following warnings for me:

.../pyoptsparse/pyoptsparse/pyIPOPT/pyIPOPT.py:28: UserWarning: pyipoptcore module could not be imported from .../repos/pyoptsparse/pyoptsparse/pyIPOPT.
  pyipoptcore = try_import_compiled_module_from_path("pyipoptcore", THIS_DIR)
.../pyoptsparse/pyoptsparse/pyNLPQLP/pyNLPQLP.py:20: UserWarning: nlpqlp module could not be imported from .../pyoptsparse/pyoptsparse/pyNLPQLP.
  nlpqlp = try_import_compiled_module_from_path("nlpqlp", THIS_DIR)

Without the changes in this PR, we always print warnings (at import time) if we cannot import these libraries. Given that they are optional and not installed by pyOptSparse, I don't think we will want this for end users. The PR changes this behaviour so that warnings are not emitted by default. Note that errors are raised at instantiation time if import fails and this PR does not change that behaviour.

@ewu63 ewu63 changed the title Bugfix/ascii chars Fix SNOPT printout encoding and skip printing import warnings Apr 22, 2024
Copy link
Contributor

@marcomangano marcomangano left a comment

Choose a reason for hiding this comment

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

LGTM, reverted the version change because we should wait for #394 before the next release #388 will trigger a minor version update

Copy link
Contributor

@sseraj sseraj left a comment

Choose a reason for hiding this comment

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

I can confirm that this fixes the non-UTF8 characters in the SNOPT print file. The import changes also look good.

@sseraj sseraj merged commit 69c2a6f into mdolab:main Apr 24, 2024
10 of 12 checks passed
@ewu63 ewu63 deleted the bugfix/ascii-chars branch April 24, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SNOPT printout contains non-UTF8 characters
3 participants