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

Rename deconvolution and identification to model_estimation #31

Closed
BjoernLudwigPTB opened this issue Feb 18, 2019 · 7 comments · Fixed by #151 or #181
Closed

Rename deconvolution and identification to model_estimation #31

BjoernLudwigPTB opened this issue Feb 18, 2019 · 7 comments · Fixed by #151 or #181

Comments

@BjoernLudwigPTB
Copy link
Member

To avoid unambiguous method naming we will combine all methods from identification and devonvolution in one new module model_estimation including the fit_filter.py methods with same namings in both modules. That requires us to rename some methods out of devonvolution.fit_filter.py. Because of the following incompatibility with previous versions of PyDynamic we need to insert a deprecation warning into the next minor release and inform users about the upcoming change.

@BjoernLudwigPTB BjoernLudwigPTB added this to To do in PyDynamic Roadmap via automation Feb 18, 2019
@BjoernLudwigPTB
Copy link
Member Author

BjoernLudwigPTB commented Jun 9, 2019

@BjoernLudwigPTB
Copy link
Member Author

@eichstaedtPTB What about identification/fit_transfer.py, should that be moved to the new module model_estimation as well or should we think about something else?

@eichstaedtPTB
Copy link
Collaborator

@eichstaedtPTB What about identification/fit_transfer.py, should that be moved to the new module model_estimation as well or should we think about something else?

This should be moved into the new module, too.

@eichstaedtPTB
Copy link
Collaborator

The methods that fit the reciprocal could be named "iLSFIR", "iLSIIR", etc.

@BjoernLudwigPTB
Copy link
Member Author

BjoernLudwigPTB commented May 27, 2020

@eichstaedtPTB What do you think about the new structure? Should we maybe have a short conversation about it or can you tell from looking at the code, that we can use this new structure for replacing identification and deconvolution?

What I did now is prefix the "old" deconvolution methods with "inv" and combine both fit_filter modules in model_estimation.fit_filter and of course transfer identification.fit_transfer into model_estimation.fit_transfer. We thought, that the proposed "i" as a prefix of "L..." would be difficult to percieve on first glance, which might lead to irritation.

I prepared the docs as well and would be grateful, if you could take a look at them and either comment or straight away contribute in the according PR #131 .

@BjoernLudwigPTB
Copy link
Member Author

BjoernLudwigPTB commented Jun 2, 2020

Introducing these changes we noticed, that we got those invLS[I,F]IR with and without uncertainties and Monte Carlo methods. We should merge these implementations and introduce optional parameters to choose one or the other.

This applies to

  • invLS[F,I]IR
  • invLS[F,I]IR_unc
  • invLSFIR_uncMC

There is no invLSIIR_uncMC since the uncertainties are determined using MC already in invLSIIR_unc .

We should then for half a year after releasing 2.0.0 backport bug fixes to the 1.4 minor releases before we stop supporting these. The docs for 2.0.0 should be enriched with the information on how to tell pip to upgrade only to the newest version of the 1.4 minor release series.

  • Besides that we actually do not fit a second-order system but rather a second-order model, thus we should rename fit_transfer.fit_sos() to fit_transfer.fit_som(). (See Rename fit_sos() to fit_som() #145 for this)

We have opened another issue for the remaining tasks. #147 .

@BjoernLudwigPTB
Copy link
Member Author

BjoernLudwigPTB commented Jun 17, 2020

What remains here is the deletion of the old modules along with the actual deprecation in v2.0.0.

@BjoernLudwigPTB BjoernLudwigPTB linked a pull request Sep 18, 2020 that will close this issue
12 tasks
@BjoernLudwigPTB BjoernLudwigPTB moved this from In progress to Reviewer approved in PyDynamic Roadmap Nov 10, 2020
PyDynamic Roadmap automation moved this from Reviewer approved to Done Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment