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

Basesolver init function now accepts a comm #30

Merged
merged 6 commits into from
Jan 7, 2021
Merged

Conversation

eirikurj
Copy link
Contributor

@eirikurj eirikurj commented Jan 7, 2021

Purpose

With the additions of self.comm in PR #29, then if the user defines self.comm in a child class before calling the BaseSolver __init__ then self.comm will get overwritten with None.

This PR adds a comm argument in the BaseSolver.__init__ function. This avoids having to define a self.comm elsewhere in child classes.
It also add a informs member variable (with a small test) as well as updates the argument list with a consistent camelCase variable.

Type of change

What types of change is it?
Select the appropriate type(s) that describe this PR

  • 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

Explain the steps needed to test the new code to verify that it does indeed address the issue and produce the expected behavior.

Checklist

Put an x in the boxes that apply.

  • I have run flake8 and black to make sure the code adheres to PEP-8 and is consistently formatted
  • 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

@eirikurj eirikurj requested a review from a team as a code owner January 7, 2021 11:24
@eirikurj eirikurj requested review from ewu63 and Xiaosong2105 January 7, 2021 11:24
marcomangano
marcomangano previously approved these changes Jan 7, 2021
ewu63
ewu63 previously approved these changes Jan 7, 2021
@sseraj
Copy link
Collaborator

sseraj commented Jan 7, 2021

Could you add a quick test for this in test_BaseSolver.py?

@ewu63
Copy link
Collaborator

ewu63 commented Jan 7, 2021

Could you add a quick test for this in test_BaseSolver.py?

I'm not sure this would make sense since that would force baseclasses to depend on mpi4py for testing. I suppose we could add a skipTest or something like that.

@sseraj
Copy link
Collaborator

sseraj commented Jan 7, 2021

It would be nice though because dependencies will break if it doesn't pass.

@eirikurj
Copy link
Contributor Author

eirikurj commented Jan 7, 2021

After speaking with @nwu63 I will add a test and few other things and finally update the other PRs.

@eirikurj eirikurj dismissed stale reviews from ewu63 and marcomangano via d239ec7 January 7, 2021 20:46
import unittest
from baseclasses import BaseSolver
from baseclasses.utils import Error


class SOLVER(BaseSolver):
def __init__(self, name, options=None, *args, **kwargs):
def __init__(self, name, options={}, comm=None, *args, **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't believe args and kwargs are used so those can probably be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will remove

@@ -1,4 +1,4 @@
__version__ = "1.2.4"
__version__ = "1.2.5"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think now that we have broken backwards compatibility, we should bump to 1.3.0 at a minimum.

Copy link
Contributor Author

@eirikurj eirikurj Jan 7, 2021

Choose a reason for hiding this comment

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

Yes I agree, will bump

@ewu63 ewu63 merged commit 4727e70 into mdolab:master Jan 7, 2021
Copy link
Collaborator

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

Thanks for adding the tests!

@@ -27,7 +27,15 @@ class AeroSolver(BaseSolver):
"""

def __init__(
self, name, category={}, def_options={}, informs={}, options={}, immutableOptions=set(), deprecatedOptions={}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think changing the order will break pyFriction so we should change that as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I will submit PR for the remaining repos that use baseclasses

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.

4 participants