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

[MNT] change cycle for renaming cINNForecaster to CINNForecaster #6120

Open
13 of 16 tasks
geetu040 opened this issue Mar 13, 2024 · 7 comments
Open
13 of 16 tasks

[MNT] change cycle for renaming cINNForecaster to CINNForecaster #6120

geetu040 opened this issue Mar 13, 2024 · 7 comments
Labels
maintenance Continuous integration, unit testing & package distribution module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting

Comments

@geetu040
Copy link
Contributor

geetu040 commented Mar 13, 2024

This issue tracks the change cycle for renaming

  • cINNForecaster to CINNForecaster in sktime.forecasting.conditional_invertible_neural_network
  • cINNNetwork to CINNNetwork in sktime.networks.cinn

The change cycle will be implemented in 3 steps with each version as follow:

  • 0.28.0

    • create aliases
      • CINNForecaster = cINNForecaster
      • CINNNetwork = cINNNetwork
    • create deprecation warning
  • 0.29.0

    • rename class
      • cINNForecaster to CINNForecaster
      • cINNNetwork to CINNNetwork
    • update deprecation message
    • switch the aliases
      • cINNForecaster = CINNForecaster
      • cINNNetwork = CINNNetwork
  • 0.30.0

    • remove aliases altogether
    • remove deprecation warning
@fkiraly
Copy link
Collaborator

fkiraly commented Mar 13, 2024

yes, that's how I would do it.

We need to check with @benHeid first whether he is ok with this renaming.

Typically, the release manager(s) make the changes directly at the release, searching for comments of the form # TODO: 0.28.0

@fkiraly
Copy link
Collaborator

fkiraly commented Mar 13, 2024

another question I'm now wondering about is why pre-commit did not pick up the name. Usually the linters are very aggressive, and I am surprised that it did not shout when a class was not UpperCamelCase.

@geetu040
Copy link
Contributor Author

geetu040 commented Mar 13, 2024

We need to check with @benHeid first whether he is ok with this renaming.

yes sure, we'll wait

pre-commit did not pick up the name.

Also I found that in documentation CINNForecaster was used instead of cINNForecaster, should it be kept as it is, but it makes more sense to use cINNForecaster in the documentation till the next release

@benHeid
Copy link
Contributor

benHeid commented Mar 16, 2024

Mhm. I would prefer to stay with the current style since the acronym cINN stands for conditional Invertible Neural Network and is mostly written cINN. However, I have no strong opinion towards this. Thus, if you prefer a renaming I wouldn't block it.

@fkiraly
Copy link
Collaborator

fkiraly commented Mar 17, 2024

Well, we would be in violation of PEP 8 and the punishment is Guido von Rossum visiting us in our nightmares:
https://peps.python.org/pep-0008/#class-names

I therefore strongly prefer UpperCamelCase. We can use the original capitalization in the docstring, for recognizability?

(we've merged the estimator already and the opposition period has elapsed, so if you feel strongly enough that it should stay so, that's how it stays, and also I will not mind)

@fkiraly fkiraly added maintenance Continuous integration, unit testing & package distribution module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting labels Mar 17, 2024
@benHeid
Copy link
Contributor

benHeid commented Mar 17, 2024

Ok then let's rename

fkiraly pushed a commit that referenced this issue Mar 22, 2024
…to `CINNForecaster` (#6121)

#### Reference Issues/PRs
This PR prepares the change cycle for renaming `cINNForecaster` to
`CINNForecaster` completing the steps for release v0.28.0 in #6120

#### What does this implement/fix? Explain your changes.
- todos on top of class definition
- warning message in class initialization
- alias line added with a todo in the bottom of the file
fkiraly pushed a commit that referenced this issue Apr 27, 2024
…ecaster` (#6238)

#### Reference Issues/PRs
This PR completes step 2 of change cycle for renaming cINNForecaster to
CINNForecaster completing the steps for release v0.29.0 in
#6120

#### What does this implement/fix? Explain your changes.
- renames the classes
- updates deprecation warning
@fkiraly
Copy link
Collaborator

fkiraly commented Apr 30, 2024

⌚ 0.29.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Continuous integration, unit testing & package distribution module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting
Projects
None yet
Development

No branches or pull requests

3 participants