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

Custom id prefixes #55

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Custom id prefixes #55

wants to merge 2 commits into from

Conversation

alex-hh
Copy link
Contributor

@alex-hh alex-hh commented Jul 25, 2022

Addresses #54 by adding kwargs to run

  • Have you added to CHANGELOG.md for this PR?

(btw I had a few dependency issues installing kotsu on my new mac - don't know if worth updating requirements to try to address? I haven't fully resolved how to make pip-compile happy, just had to manually install stuff).

@DBCerigo DBCerigo requested review from DBCerigo and ali-tny August 16, 2022 10:29
Copy link
Contributor

@DBCerigo DBCerigo left a comment

Choose a reason for hiding this comment

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

Implementation wise, I think all looks great. The only bit bugging me is the arg names, validation_prefix and model_prefix. I think they are a tad misleading, i.e. they indicate on first reading adding a prefix to the validations IDs, as opposed to setting the name for the validation-esq thing. Getting me on that? I think we could do better. Some ideas:

  • changing validation and model everywhere to validator and validatee (I had already been thinking about making the names more general that model etc. given you could the use package outside of data science also, maybe overkill though, probs better names than the one I suggested here also), then arg names could be validator_rename, validatee_rename
  • validation_rename, model_rename
  • rename_validation, rename_model
  • validation_entity_rename, model_entity_rename

(btw I had a few dependency issues installing kotsu on my new mac - don't know if worth updating requirements to try to address? I haven't fully resolved how to make pip-compile happy, just had to manually install stuff).

Yea I just been thinking about that in #53 - maybe we shouldn't be using a compiled ...txt in dev? People just use whatever in dev, and then we always get the latest versions of deps in the CI.

@ali-tny
Copy link
Contributor

ali-tny commented Aug 24, 2022

the naming thing is super difficult... i think "prefix" and "rename" sound like they're altering the actual name of each model

likewise "validatee" i think is a bit weird

is it a huge problem for sktime that kotsu calls them validators and models? is it a possibility for us to just not name them like that?

an alternative that i find slightly less weird wrt naming is doing something like allowing users to set a entity_type: str attribute or something on the registry itself (that defaults to "validation" or "model"), and then use that name to form the column names?

@DBCerigo
Copy link
Contributor

@alex-hh got any further thoughts on this?

I wouldn't want to move from "validation" to "benchmarks" - I think "benchmarks" is overloaded, as it could be the process (the validation) or the result.

The rest of the ideas I'm open to.

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.

3 participants