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 or Remove Default Simulation Params #222

Closed
nagakingg opened this issue Sep 6, 2023 · 2 comments · Fixed by #247
Closed

Fix or Remove Default Simulation Params #222

nagakingg opened this issue Sep 6, 2023 · 2 comments · Fixed by #247
Assignees
Labels
bug Something isn't working
Milestone

Comments

@nagakingg
Copy link
Collaborator

nagakingg commented Sep 6, 2023

Version Information

curvesim 0.5.0.b1; all OSs and Python versions

What's your issue about?

This bug was user-reported, and I verified it.

Running a v2 autosim with no parameter input raises a ParameterSamplerError:

res = curvesim.autosim(
    "0x5426178799ee0a0181a89b4f57efddfab49941ec",
    env="staging",
)

curvesim/templates/param_samplers.py", line 129, in _validate_attributes
    raise ParameterSamplerError(
curvesim.exceptions.ParameterSamplerError: Input parameters not found in 'ParameterizedCurveCryptoPoolIterator.setters' or 'SimCurveCryptoPool' attributes: ['fee']

How can it be fixed?

This is caused by the v1 parameter defaults being applied to the v2 pool in both pipelines pipeline functions:

    default_params = DEFAULT_PARAMS.copy()
    for key in DEFAULT_PARAMS:
        if key in fixed_params:
            del default_params[key]

    variable_params = variable_params or DEFAULT_PARAMS

default_params = DEFAULT_PARAMS.copy()

default_params = DEFAULT_PARAMS.copy()

There are two options:

  • make default parameters for each pool type
  • remove the default parameters altogether

My sense is that we should simply remove the default parameters (and the above code block that applies them) since they are rarely/never used. This would mean that running a pipeline with no parameter kwargs would run one run with the pool's fetched parameters. This would require a minor update to the readme/documentation.

In the future we might include some standard settings to choose from, but I think they could be defined in a separate submodule and shouldn't be applied by default.

@chanhosuh
Copy link
Member

Let's remove autosim and also the test option. That can be explicitly chosen and passed in for tests.

@nagakingg
Copy link
Collaborator Author

nagakingg commented Sep 6, 2023

Noting the results of our discussion:

Immediate change:
Remove default and test parameters. This issue can be closed with this completed.

Subsequent changes:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants