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

Advanced amp model .dgt vector reversal #390

Closed
cgkelly opened this issue Apr 8, 2021 · 10 comments
Closed

Advanced amp model .dgt vector reversal #390

cgkelly opened this issue Apr 8, 2021 · 10 comments

Comments

@cgkelly
Copy link

cgkelly commented Apr 8, 2021

The example .dgt vector in the Juniper std_medium_gain_advanced_config.json file appears to be reversed.
The documentation on how to use is sparse, and the code does not appear to account for the possible
optimization of the amp design for minimum ripple at a tilt value other than zero.

  1. Current Usage: The .dgt vector appears to define the gain ripple as a function of tilt:
    Ripple = .dgt x tilt. To accommodate amplifiers that are designed for minimum ripple at a non zero tilt,
    this should be modified to ripple = .dgt x (tilt-flat gain tilt).

  2. The sign convention for amplifier tilt needs to be defined/documented. My assumption is fiber tilt due to SRS is positive (lower loss or positive "gain" at higher wavelengths, thus, to counteract this, amplifiers require a negative tilt).

  3. In trying to understand how to use the .dgt vector, it became apparent that the example vector is reversed.
    The attached slides illustrate this, and also help clarify how this vector is derived and its usage.
    It also suggests that if unknown, a default set of vector coefficients can be derived, as the expected shape from
    simple fixed gain C band EDFA simulations match the (reversed) Juniper .dgt vector quite well, once any intrinsic
    tilt within the .dgt vector itself is removed.

GNPY_dgt_v1.pptx

@ojnas
Copy link
Contributor

ojnas commented Apr 8, 2021

@cgkelly, I believe your suspicion is correct that the dgt vector in std_medium_gain_advanced_config.json (the Juniper-BoosterHG.json file seems to have the same problem) was originally given vs. wavelength. The dgt implementation was based on a proposal from Ciena and their original slides had a figure with DGT vs. wavelength. For reference, the first implementation (at least that I could find) is here:

https://github.com/Telecominfraproject/oopt-gnpy/blob/58ac717f8dcf8795d336f56cef52cb60a3729380/examples/edfa_model/amplifier.py

About gain tilt when operating in the extended gain regime, it was discussed here:

#240

@ojnas
Copy link
Contributor

ojnas commented Apr 9, 2021

@ggrammel, about the dgt values (and gain_ripple) in Juniper-BoosterHG.json, do you know for what frequencies they were measured and specifically whether they are given in increasing or decreasing frequency order?

(This file also has another problem: there are 48 dgt values, which I guess were measured on a 100 GHz grid. But still it specifies "f_min": 191.35e12, which means the frequency values used for interpolation in interpol_params in elements.py will be slightly offset.)

@ojnas
Copy link
Contributor

ojnas commented Apr 9, 2021

Just to clarify, even though this issue is titled "Advanced amp model ..." and refers to std_medium_gain_advanced_config.json, the same dgt values are used in default_edfa_config.json, which is used by all other amplifiers models. This means all amplifiers will have wrong gain vs. frequency values whenever a non-zero tilt_target is specified.

@ggrammel
Copy link
Collaborator

@ojnas attached the input data for the HGAmp. Note that the input data was measured in steps of 100GHz, while the converter script required 50GHz steps
Archive.zip

@ojnas
Copy link
Contributor

ojnas commented Apr 21, 2021

@cgkelly, all, I have pushed three commits to gerrit which address some of the issues:

https://review.gerrithub.io/c/Telecominfraproject/oopt-gnpy/+/514993
https://review.gerrithub.io/c/Telecominfraproject/oopt-gnpy/+/514994
https://review.gerrithub.io/c/Telecominfraproject/oopt-gnpy/+/514995

Please have a look if the changes make sense and let's continue discussing the other remaining issues.

@cgkelly
Copy link
Author

cgkelly commented Apr 21, 2021 via email

jktjkt pushed a commit that referenced this issue Apr 22, 2021
As identified in GitHub issue #390, the dgt values (as well as gain and
nf ripple values) in example config json files are listed in order of
increasing wavelength (decreasing frequency) while the code assumes
values listed in the opposite order. This patch reverses the order of
values in affected files so that they are consistent with existing use
in the code.

Also, the f_min value in the Juniper-BoosterHG.json file is updated to
match measurement data so that interpolation is performed correctly.

Change-Id: I97a9d2f9be81380d1658bee5fa1ef4def3e1c537
Signed-off-by: Jonas Mårtensson <jonas.martensson@ri.se>
jktjkt pushed a commit that referenced this issue Apr 22, 2021
As pointed out in GitHub issue #390, the normal convention for the sign
of amplifier tilt is to define it with regard to wavelength, i.e.
negative tilt means lower gain for longer wavelengths (lower
frequencies). Currently GNPy uses the opposite convention, which this
patch proposes to change.

Signed-off-by: Jonas Mårtensson <jonas.martensson@ri.se>
Change-Id: I8f7829a3b0b0b710f7da013c525630a60b22a2b5
@jktjkt
Copy link
Collaborator

jktjkt commented Apr 22, 2021

@cgkelly , thanks for the review and for catching this in the first place. @ojnas , thanks for the fix -- just merged.

@jktjkt jktjkt closed this as completed Apr 22, 2021
@ojnas
Copy link
Contributor

ojnas commented Apr 27, 2021

@cgkelly, I'm thinking about implementing your suggestion to accommodate amplifiers designed for minimum ripple at non-zero tilt. This would require an additional amplifier parameter in the equipment input file defining this non-zero tilt value. Is there a commonly accepted name for this parameter (I think I've heard pre-tilt and built-in tilt)? Also, do you think the tilt_target defined in the topology input file should be an offset from this non-zero tilt or should it be the actual total tilt?

@cgkelly
Copy link
Author

cgkelly commented Apr 27, 2021 via email

@cgkelly
Copy link
Author

cgkelly commented Apr 29, 2021 via email

jktjkt pushed a commit that referenced this issue Sep 14, 2022
This was corrected for example-data but not for tests data
(in commit 3a72ce8, related
to issue #390)

Signed-off-by: EstherLerouzic <esther.lerouzic@orange.com>
Change-Id: I929beeb034166d30aa994439a1d6a26350f5c3e9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants