ietf-ccamp-wg / draft-ietf-ccamp-optical-impairment-topology-yang

8 stars 10 forks source link

Try to shorten the names of attributes #134

Closed sergiobelotti closed 9 months ago

sergiobelotti commented 1 year ago

Related to https://github.com/ietf-ccamp-wg/ietf-ccamp-layer0-types-ext-RFC9093-bis/issues/69, the intent is to analuze the modules and try to shorten the names of attributes that appears too long, without loosing the meaning of the attribute.

sergiobelotti commented 1 year ago

Looking at the tree it seems that the attributes more critical are:

EstherLerouzic commented 1 year ago

then we could use pmd-penalty and pdl-penalty

pmd and pdl are well known acronyms and description has the definition

italobusi commented 1 year ago

Looking at the tree it seems that the attributes more critical are:

Just one minor doubt: should it be pdl-penalty or max-pdl-penalty?

sergiobelotti commented 1 year ago

Unfortunately to shorten same names of attributes does not solve the issue: Dieter showed me the .txt of version 13, with simulation importing the actual last tree related to PR#135 and there are 4 lines exceeding 72 line length all with the types field. This is due to the 3 characters more added by the compilation from XML to txt . For example: +--ro nominal-carrier-power? | | | l0-types:power-in-dbm-or-null editing the tree with normal editor we are still within 72 char, but txt process adding 3 more characters creating the issue. Obviously we can partially mitigate the problem shortening some typedef names.

italobusi commented 1 year ago

IMHO, for the two attributes mentioned above, the new names by proposed by Esther are better regardless of the line length issue

For the I-D submission, since there are only 4 lines, I would suggest to fix the issue manually and then to raise the issue with Netmod WG

One possibility could be to fold the YANG tree using the tool defined in RFC8792 but this option is currently precluded by the scope of RFC8792 based on the assumption that pyang can solve the line length issue which seems not correct based on our experience

manzoro commented 11 months ago

As per our discussion on IETF-118 side meeting, I have analyzed the lines still with with >69 columns (by using rfcfold and double checking manually); the problem is mostly due to the tree indentation (as per Italo/Dieter observation on July 5) and cannot be fixed by just shortening the attribute name. The fix may require to shorten some definition in L0-types.

For now I've not created yet a PR branch to work on it, I'm reporting here the affected lines that are all within the 'amplifier-element' section and highlighted in this comment, lets discuss together how to move forward in the next call.

                |           +--ro (power-param)?
                |           |  +--:(channel-power)
                |           |  |  +--ro nominal-carrier-power?
                |           |  |          l0-types:power-in-dbm-or-null            <-- exceed by 2 chars
                |           |  +--:(power-spectral-density)
                |           |     +--ro nominal-power-spectral-density?            <-- exceed by 2 chars
                |           |             l0-types:decimal-16-digits-or-null       <-- exceed by 7 chars
                |           +--ro raman-direction?
                |           |       enumeration
                |           +--ro raman-pump* []
                |           |  +--ro frequency?
                |           |  |       l0-types:frequency-thz
                |           |  +--ro power?
                |           |          l0-types:decimal-2-digits-or-null           <-- exceed by 3 chars
                |           +--ro pdl?
                |           |       l0-types:loss-in-db-or-null
                |           +--ro media-channel-groups
                |              +--ro media-channel-group* []
                |                 +--ro media-channels* []
                |                    +--ro flexi-n?
                |                    |       l0-types:flexi-n
                |                    +--ro flexi-m?
                |                    |       l0-types:flexi-m
                |                    +--ro delta-power?
                |                            l0-types:gain-in-db-or-null           <-- exceed by 3 chars
sergiobelotti commented 11 months ago

@manzoro : thanks Roberto for the analysis. One question for clarification: when you say "The fix may require to shorten some definition in L0-types." you mean some typedef definitions or again to shorten some attributes name? I do not see any other method to fix the problem . Some data nodes used in Optical Impairment model are defined in L0-types and so again we need to fix the problem there.

manzoro commented 11 months ago

@sergiobelotti : I mean that we may need to shorten some typedef in L0-types like for example "decimal-16-digit-or-null".

The only line that can be fixed with an attribute name change is the second one. All the other are exceeding the 69 column simply because of the tree indentation and can only fit if the typedef in L0-types is shorter.

sergiobelotti commented 11 months ago
+--ro (power-param)?
                |           |  +--:(channel-power)
                |           |  |  +--ro nominal-carrier-power?
                |           |  |          l0-types:power-in-dbm-or-null            <-- exceed by 2 chars
dbm-or-null
                |           |  +--:(power-spectral-density)
                |           |     +--ro nominal-power-spectral-density?            <-- exceed by 2 chars
nom-power-spectral-density
                |           |             l0-types:decimal-16-digits-or-null       <-- exceed by 7 chars
psd-or-null
                |           +--ro raman-direction?
                |           |       enumeration
                |           +--ro raman-pump* []
                |           |  +--ro frequency?
                |           |  |       l0-types:frequency-thz
                |           |  +--ro power?
                |           |          l0-types:decimal-2-digits-or-null           <-- exceed by 3 chars
dec-2-digit-or-null
                |           +--ro pdl?
                |           |       l0-types:loss-in-db-or-null
                |           +--ro media-channel-groups
                |              +--ro media-channel-group* []
                |                 +--ro media-channels* []
                |                    +--ro flexi-n?
                |                    |       l0-types:flexi-n
                |                    +--ro flexi-m?
                |                    |       l0-types:flexi-m
                |                    +--ro delta-power?
                |                            l0-types:gain-in-db-or-null           <-- exceed by 3 chars
it is 
sergiobelotti commented 11 months ago

Call on 11/21/23: based on Roberto's analysis, we made some proposals to solve the various exceeded cases.

l0-types:power-in-dbm-or-null --> dbm-or-null proposal: "dbm" is used just for power so no need to put power in the typedef.

nominal-power-spectral-density --> nom-power-spectral-density or, since "psd" can stand for power spectral density , an alterantive can be --> nominal-psd

l0-types:decimal-16-digits-or-null --> we have already changed in l0-types into "power-spectral-desnsity-or-null" since decimal-16-digit-or-null is used only for power spectral density. So to shorten the type --> psd-or-null

l0-types:decimal-2-digits-or-null --> dec-2-digit-or-null

l0-types:gain-in-db-or-null --> have already been changed into ratio-in-db-or-null

We agreed do not change that since more clear "ratio-in-db-or-null" Julien/Esther: we do not risk to loose clarity in the attempt to solve problem of indentation.

egriseri commented 11 months ago

Hi @sergiobelotti I agree on the proposal. As additional comment I think nominal-psd is sleeker and easier to read than nom-power-spectral-density and more coherent with psd-or-null.

italobusi commented 11 months ago

Call on 11/21/23: based on Roberto's analysis, we made some proposals to solve the various exceeded cases.

l0-types:power-in-dbm-or-null --> dbm-or-null proposal: "dbm" is used just for power so no need to put power in the typedef.

I understand that also power-in-dbm --> dbm, right?

In general, I think all the foo datatypes will have a foo-or-null counterpart following the same abbreviation

The use of dbm as a data type is a bit inconsistent with other cases where we define the data type based on what it measured rather than based on the measurement unit, but power can be measured in dbm or in Watt (there is an attribute where power is measured in Watts) SB> yes , the comment is absolutely correct. We need to have a "unique" philosophy to define the different types and till now we always preferred to make definition related to the entity measured rather than the measurement unit.

What about defining power-dbm and power-watt instead? SB> power-dbm would be ok for me. For power-watt I'm wondering if it is the case to define a type for a single attribute (power in the pump) , that is defined as decimal-2-digits-or-null, type that is used in other attributes that are nothing to do with power.

Alternatively (less preferred), pwr-dbm or pwr-watt.

nominal-power-spectral-density --> nom-power-spectral-density or, since "psd" can stand for power spectral density , an alterantive can be --> nominal-psd

I also prefer nominal-psd SB> I'm OK

l0-types:decimal-16-digits-or-null --> we have already changed in l0-types into "power-spectral-desnsity-or-null" since decimal-16-digit-or-null is used only for power spectral density. So to shorten the type --> psd-or-null

l0-types:decimal-2-digits-or-null --> dec-2-digit-or-null

I do not have a strong opinion but I am not fully sure how common is dec used as an abbreviation for decimal ...

What about decimal-2? SB> OK

Alternative (less preferred), decimal-2dp (decimal places, although not sure how much common is the dp abbrevation)?

l0-types:gain-in-db-or-null --> have already been changed into ratio-in-db-or-null

My understanding is that we are just adding ratio-in-db and keep gain-in-db and loss-in-db

What about changing them to power-ratio, power-gain and power-loss? See initial comment SB> no objection

Alternatively (less preferred): pwr-ratio, pwr-gain and pwr-loss.

We agreed do not change that since more clear "ratio-in-db-or-null" Julien/Esther: we do not risk to loose clarity in the attempt to solve problem of indentation.

Just my 2 cents

@italobusi : do you think can we have BC issues with these changes with respect modules importing the old RFC9093 ?

sergiobelotti commented 11 months ago

Hi @sergiobelotti I agree on the proposal. As additional comment I think nominal-psd is sleeker and easier to read than nom-power-spectral-density and more coherent with psd-or-null.

@egriseri Thanks Enrico, I agree on you preference about nominal-psd

italobusi commented 11 months ago

SB> power-dbm would be ok for me. For power-watt I'm wondering if it is the case to define a type for a single attribute (power in the pump) , that is defined as decimal-2-digits-or-null, type that is used in other attributes that are nothing to do with power.

IB> Not a strong opinion from me. I was just thinking about a possible comment asking why power-dbm and not just power as in other cases

@italobusi : do you think can we have BC issues with these changes with respect modules importing the old RFC9093 ?

IB: I think so because, if I recall well, we are changing definitions which were not in RFC9093. However, it is worthwhile checking that all the changes we are doing are BC with RFC9093. I have created issue ietf-ccamp-wg/ietf-ccamp-layer0-types-ext-RFC9093-bis#83 not to forget: I do not know which label to assign to it to indicate that this is something to do before WG LC

sergiobelotti commented 11 months ago

SB> power-dbm would be ok for me. For power-watt I'm wondering if it is the case to define a type for a single attribute (power in the pump) , that is defined as decimal-2-digits-or-null, type that is used in other attributes that are nothing to do with power.

IB> Not a strong opinion from me. I was just thinking about a possible comment asking why power-dbm and not just power as in other cases

@italobusi : do you think can we have BC issues with these changes with respect modules importing the old RFC9093 ?

IB: I think so because, if I recall well, we are changing definitions which were not in RFC9093. However, it is worthwhile checking that all the changes we are doing are BC with RFC9093. I have created issue ietf-ccamp-wg/ietf-ccamp-layer0-types-ext-RFC9093-bis#83 not to forget: I do not know which label to assign to it to indicate that this is something to do before WG LC

What about "before-WG LC" as label ?

ju7ien commented 11 months ago

SB> power-dbm would be ok for me. For power-watt I'm wondering if it is the case to define a type for a single attribute (power in the pump) , that is defined as decimal-2-digits-or-null, type that is used in other attributes that are nothing to do with power.

IB> Not a strong opinion from me. I was just thinking about a possible comment asking why power-dbm and not just power as in other cases

I think there's no value in creating a new unit type for power-watt and decimal-2-digits-or-null is perfectly fine.

manzoro commented 11 months ago

@italobusi and @sergiobelotti: I've seen the commits applied in wg-lc-prep as per the discussion here, I have one more proposal to remove one additional long line: we replaced power-spectral-density with psd in the attributes names, why don't do it also in the L0types for consistency so that we get change of

                |           |     +--:(power-spectral-density)
                |           |        +--ro nominal-psd
                |           |                l0-types:power-spectral-density-or-null

into

                |           |     +--:(power-spectral-density)
                |           |        +--ro nominal-psd
                |           |                l0-types:psd-or-null