Strings like 'l2' and 'l1' would be preferable as 'L2' and 'L1"

[woodsp-ibm]

Strings like 'l2' and 'l1' would be preferable as 'L2' and 'L1"

[adekusar-drl]

I agree in general, but we have other loss functions coded as lower case strings: cross_entropy, cross_entropy_sigmoid, so I think we need a consistent solution. I suggest replace l1 with absolute_error and l2 with squared_error.
@beichensinn what do you think?

[woodsp-ibm]

The strings across the API are case insensitive as it does a lower() on what is passed in to compare against constants in the code in lowercase. If upper() was done instead it would just mean the constants all became upper internally and maybe we'd show them that way in the docstring too. It would not change the API in this case; but changing to different strings means deprecating the l1 and l2 so they'd have to stay in the code for a while. Of course doing this upper() instead, and having it as 'L1' internally and in the docs, the API, would not prevent users from using the 'l1' etc in lowercase in their code and thus have the potential readability problem there still, that different strings would prevent.

[mattwright99]

@woodsp-ibm @adekusar-drl I am new to qiskit contributions but I am interested in picking up this issue. Would that be ok?

I think using upper() and comparing to 'L1' internally is a good idea since it would make the code more readible and the docstrings more clear. I think it could also be beneficial to add support for 'absolute_error' and 'squared_error' in addition to 'L1' and 'L2' respectively.
I also saw a comment in #92 suggesting that we should extract the if...else block to a seperate class and I could see that being in the scope of this PR. Do you agree? If so, could you provide some more information on that (e.g. what class should I look at)?
Thanks!

[adekusar-drl]

@mattwright99 Surely, you can work on this issue!
I'd prefer to keep string constants in lower case just to keep them more readable in the documentation. I suggest to gracefully deprecate l1 and l2 in favor of 'absolute_error' and 'squared_error' cause this would solve the issue completely.
The if block you mentioned, is it the if block where we instantiate a loss object out of a string value? If so, then I happy to discuss your suggestion here, but please don't code it upfront.

[mattwright99]

@adekusar-drl Alright thanks, i am excited to start contributing! I will let you know if I have any questions!

[adekusar-drl]

@manoelmarques we want to deprecate certain values passed to the TrainableModel constructor, e.g. instead of loss="l2" we'd like to have loss="squared_error" and deprecate l2. I know you've added a few functions for handling deprecations, but I'm not sure how they can be applied to argument values. Is there an example of such deprecation somewhere?

[manoelmarques]

@adekusar-drl I added all the possible ways you can use deprecations in the test_deprecation.py. However All methods/decorators expect you to deprecate a class, method, argument. property or function. Nothing for default values. The message being created automatically doesn't support it. Would it be possible for you to add a warnings.warn method in your PR for that situation and I will then create some method to support it and use it in place, either in the same PR or another ?

[adekusar-drl]

@manoelmarques Yes, we can generate manual warnings.
@mattwright99 Please add lines like these:

warnings.warn(
    "The 'l2' value is deprecated as of 0.2.0 "
    "and will be removed no sooner than 3 months after the release. "
    "You should use the 'squared_error' value instead.",
    DeprecationWarning,
)

The version number in the text depends on the date when you'll be able to finish the PR. We can edit it later on.

[mattwright99]

@adekusar-drl sounds good, I am just about to publish a PR