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

BUG-FIX: added kwargs to plugins #122

Merged
merged 6 commits into from
Feb 7, 2023
Merged

Conversation

gsel9
Copy link
Contributor

@gsel9 gsel9 commented Feb 1, 2023

A small fix of passing kwargs to the uniform_sampler and marginal_distirbutions plugins that can be useful (at least to me). After changing the src code, I re-installed the package and successfully ran the relevant tests.

@bcebere
Copy link
Contributor

bcebere commented Feb 1, 2023

@gsel9 Thank you for your contribution!

Please fix the test errors, and I will merge the PR! Thanks!

@@ -33,7 +33,7 @@ repos:
"--extend-ignore=E203,W503"
]
- repo: https://github.com/pre-commit/mirrors-mypy
rev: v0.812
rev: v0.991
Copy link
Contributor

Choose a reason for hiding this comment

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

mypy isn't the issue, this version shouldn't be changed.

The errors are from black.

Please revert the mypy version and run

pre-commit install
pre-commit run --all

it should be clear where the error is after that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Long story short, it fixed the black issues on my end (I am a Mac user). However, I also raised a lot of other fixes when running the pre-commit tests locally. Is there a reason for using this specific version of mypy?

I will surely revert the mypy version and try to solve my orginal problem in a different way.

@@ -35,7 +35,7 @@ class MarginalDistributionPlugin(Plugin):

def __init__(self, **kwargs: Any) -> None:
super().__init__(
sampling_strategy="marginal",
sampling_strategy="marginal", **kwargs
Copy link
Contributor

Choose a reason for hiding this comment

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

On my end it shows that black reformats this line to

super().__init__(sampling_strategy="marginal", **kwargs)

I am not sure why black isn't doing the same on MacOS, since the version is pinned in the .pre-commit config.

@bcebere
Copy link
Contributor

bcebere commented Feb 6, 2023

I updated the .pre-commit tools on the main branch, if that helps.

@bcebere
Copy link
Contributor

bcebere commented Feb 7, 2023

It looks like tests\plugins\time_series\test_ts_marginal_distributions.py passed an unused parameter n_iter, which is not working with your changes.

Feel free to remove the n_iter occurrences in tests\plugins\time_series\test_ts_marginal_distributions.py. Thanks!

Copy link
Contributor

@bcebere bcebere left a 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 contribution! LGTM!

@bcebere bcebere merged commit 48b7abf into vanderschaarlab:main Feb 7, 2023
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.

2 participants