-
Notifications
You must be signed in to change notification settings - Fork 9
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
dropped ensemble requirement to single model #69
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contributions. If you haven't already, please send a signed Contributor License Agreement (CLA) to Christopher Teubert ([email protected]). CLAs can be found here: https://github.com/nasa/prog_models/tree/master/forms. Also, make sure you're familiar with the developer notes and contributing sections of our developers guide, https://nasa.github.io/progpy/dev_guide.html#notes-for-developers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Ashwani132003 for your PR. A few suggestions before I can approve it. Let me know if you would like help with any of this.
src/progpy/ensemble_model.py
Outdated
if len(models) < 2: | ||
raise ValueError('EnsembleModel requires at least two models') | ||
if len(models) < 1: | ||
raise ValueError('EnsembleModel requires at least one models') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raise ValueError('EnsembleModel requires at least one models') | |
raise ValueError('EnsembleModel requires at least one model') |
tests/test_ensemble.py
Outdated
try: | ||
EnsembleModel([m]) | ||
except ValueError: | ||
self.fail("Error EnsembleModel raised ValueError Unexpectedly") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try: | |
EnsembleModel([m]) | |
except ValueError: | |
self.fail("Error EnsembleModel raised ValueError Unexpectedly") | |
EnsembleModel([m]) |
Try-except not necessary - test would fail if value error is raised
Follow-up note: ignore the failing tests. Tests are not working properly for some reason on this branch. For this PR I will run the tests manually |
e4598c3
to
e57af59
Compare
Hi @Ashwani132003, I'd be happy to help with your PR. Can you update me on the current status? Are you still having trouble with the extra/previous commits alongside yours? |
hey, thanks for asking, but I somehow managed to resolve that, you can tell me if there are any other issues related to my pr. |
Hi @Ashwani132003, I'm glad you got the previous issues resolved. I'm reviewing your code and it looks good so far. I'm having trouble pulling your code, and I think it's because it's forked off of dev. Will you please branch off of the ProgPy dev branch, add the changes to that new branch, and then request a review from me and @teubert? Then we should be able to review and merge. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay. I was on leave last week, but now I'm back.
Everything looks great. Thank you for your help with this.
Close #11