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

treewide: Cleanup. Reduce the number of warnings issued during test execution #2541

Merged
merged 12 commits into from
Nov 22, 2022

Conversation

jvesely
Copy link
Collaborator

@jvesely jvesely commented Nov 18, 2022

Update patterns that cause Python warnings at runtime

  • Rewrite Angle function algorithm to avoid division and improve performance. Fixes division by zero during initialization.
  • Remove the use of np.float. The existing use cases already consider basic float, np.float is just a deprecated alias of that.
  • Update TransferMechanism tests that override parameters to capture and check for the presence of override warning
  • Update tests that hardcode checks for warnings in a specific order
  • Adjust test configurations to avoid warnings unless the test is intended to exercise situations that produce warnings
  • Use both python 3.7 and python 3.8+ codepaths in AST parser

It's an obsolete alias to 'float', use that instead.

Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu>
Original time: 247 usecs.
Optimizations:
 * Use numpy array operations for sin, these are faster than Python iterations. Time: 214 usecs
 * Use numpy array operations for sin and cos, these are faster than Python iterations. Time: 191 usecs
 * Use numpy cumsum instead of complete product with partial divisions,
   this is both faster and more accurate. it also avoids potential issue
   with division by zero. Time: 180 usecs

Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu>
…iable

Avoids zero values in distance computation during initialization.

Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu>
Empty list creates empty elementwise comparison which is false and
deprecated.
Add an explicit check to avoid that.

Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu>
… if available

The latter are deprecated in Python 3.8+.

Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu>
…n construction

The latter is deprecated.

Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu>
No reason to test the construction again with no execution.
Simplify generation of test names.

Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu>
…nflict between mech and function parameter

Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu>
It's not used in the test and PsyNeuLink warns about it.

Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu>
…self

This is a common situation in tests that assign the same object to e.g.
default variable, and then variable for execution.

Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu>
@github-actions
Copy link

This PR causes the following changes to the html docs (ubuntu-latest-3.7-x64):

No differences!

...

See CI logs for the full diff.

@jvesely
Copy link
Collaborator Author

jvesely commented Nov 18, 2022

This removes most "DeprecationWarning:" messages (GA jobs go from ~36k to ~17k). The ones that remain are:

UserWarnings were reduced to (when running -m llvm):

  • missing projection to input port. This is a bug the mechanism function doesn't take input so PNL shouldn't be complaining about a missing projection.
  • default parameters used for GridSearch
  • Compiler shape mismatch warnings.

@jvesely
Copy link
Collaborator Author

jvesely commented Nov 18, 2022

@kmantel do you mind looking at the change to is_compatible?
@jdc do you mind looking at the change to the Angle implementation?

@@ -440,7 +440,8 @@ def iscompatible(candidate, reference=None, **kargs):
try:
with warnings.catch_warnings():
warnings.simplefilter(action='ignore', category=FutureWarning)
if reference is not None and (candidate == reference):
# np.array(...).size > 0 checks for empty list. Everything else create single element (dtype=obejct) array
if reference is not None and np.array(candidate, dtype=object).size > 0 and (candidate == reference):
Copy link
Collaborator

Choose a reason for hiding this comment

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

No issue with this, but we could potentially also use

Suggested change
if reference is not None and np.array(candidate, dtype=object).size > 0 and (candidate == reference):
if reference is not None and np.array(candidate, dtype=object).size > 0 and safe_equals(candidate, reference):

although safe_equals would need a couple small changes because I think the type of warning produced by that == check has changed since it was written

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had the same idea [0,1], but it gave enough errors to leave it for the next time.
Most of the remaining warnings (in total number) are from the elementwise comparison and np.matrix so it's definitely worth revisting.

[0] https://github.com/jvesely/PsyNeuLink/actions/runs/3492618053
[1] https://ci.appveyor.com/project/jvesely/psyneulink-cuda/builds/45417908/job/m8adfgaec8pfs27u

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ok, makes sense. I can make those changes separately then, they're not many.

@github-actions
Copy link

This PR causes the following changes to the html docs (ubuntu-latest-3.7-x64):

No differences!

...

See CI logs for the full diff.

@github-actions
Copy link

This PR causes the following changes to the html docs (ubuntu-latest-3.7-x64):

No differences!

...

See CI logs for the full diff.

@jvesely jvesely merged commit 50c545b into PrincetonUniversity:devel Nov 22, 2022
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.

None yet

2 participants