Build no amp in roadm and add roadm restrictions on amps #230

EstherLerouzic · 2019-05-17T16:25:19Z

This PR adds the amp restriction in Roadms (booster and preamp).
to use it with xls input file
-user may enter (valid) restrictions in eqpt_config.json and all roadms will have same restriction
-user may add per Roadm restriction in Nodes sheet (column 9 and 10 for booster and preamp, respectively). this will override eqpt_config general input for this particular node
to use it with json:

user may enter (valid) restrictions in eqpt_config.json and all roadms will have same restriction
user must add a valid restriction field in the json roadm element. this will override general input
Note that restrictions apply to the entire roadm, and not per degree.

a special case without booster is enabled via the xls eqpt sheet: if a Roadm amp is marked fused, this will add a Fused element instead of an amp, enabling special design of roadms without booster. This can be applied per degree

This PR refers to #211 discussion. @jeanluc-auge, @ojnas , @jktjkt could you please have a look and tell me how to improve ? Thanks !

jktjkt

Hi Esther, inline are some comments and nitpicks on how I would do things differently to increase readability a bit. Feedback is very welcome. I'll be looking into the actual code next week.

gnpy/core/elements.py

gnpy/core/network.py

tests/data/eqpt_config.json

EstherLerouzic · 2019-05-23T06:58:55Z

Please do not merge yet: I am still working on the associated tests

gnpy/core/equipment.py

gnpy/core/network.py

gnpy/core/utils.py

jktjkt · 2019-05-23T23:35:18Z

gnpy/core/utils.py

+
+    >>> d3 = {'params': {'restrictions': {'preamp_variety_list': ['foo'], 'booster_variety_list': ['bar']}}}
+    >>> merge_dicts_recursively(d1, d3)
+    {'params': {'restrictions': {'preamp_variety_list': [], 'booster_variety_list': []}}}


@EstherLerouzic , can you please confirm that this is a correct behavior? I don't know whether it would make more sense to merge the leaf lists or not. Either way, this test just documents the current behavior.

Thanks for the commits !
This is the correct behaviour. Restriction read from node overrides restriction from eqpt_config.json if they exist, so we should not merge preamp and booster lists.
so maybe the naming is not correct, it is more an update

Please feel free to squash my changes to the commits which introduce that code. Or not, I do not really have a preference here.

Also feel free to rename this function if you feel like doing that. Perhaps something like merge_amplifier_restrictions?

There are no doctests right now, but they will (might?) be added (see PR Telecominfraproject#230).

jktjkt · 2019-05-24T13:54:48Z

And just FYI, it would be a bit easier for me if the work is split into small commits, each doing only one thing. This commit 038fe18 includes both trivial whitespace changes and replacing prints by logging plus exceptions. If these were separate, I would be able to glance over the trivial whitespace fixes very quickly, and focus on the more important changes.

If this is not a big problem for you, please consider splitting your work and committing more often.

jktjkt · 2019-05-27T14:48:49Z

gnpy/core/utils.py

+    """Raised when the json equipment is not formed correctly
+    """
+    import sys
+    sys.tracebacklimit = 0


I do not think that this is a nice coding pattern. Raising an exception should not result in a random callback into the sys module and trying to reconfigure backtrace behavior. Also, this variable is no longer available in Python 3.

If the intention is to suppress detailed error messages, then I suggest moving any UI-affecting bits into the CLI driver, i.e., examples/transmission_main_example.py. I'll create another pull request for these exceptions.

EstherLerouzic · 2019-05-31T09:05:17Z

hi @jktjkt ,
Is there anything else I should do for this PR to be at the level needed to be merged in develop ?

jktjkt · 2019-05-31T09:36:05Z

Is there anything else I should do for this PR to be at the level needed to be merged in develop ?

I would like to have these two addressed before this gets merged:

Let's use exceptions and the associated error handling added in Use exceptions instead of sys.exit() when reporting configuration errors #251. I think there are two possibilities:
- backporting this series on top of recent develop
- doing that via git merges and resolving conflicts

Either you or me can do either of these two, please tell me what your preference is.

Depend on that -15 dBm target power for transceivers which is done in correct test and example files with direct Trx reception #244. But I do not want correct test and example files with direct Trx reception #244 to be merged right away because it is not covered by tests. Again, two options IMHO;
- wait for me to get correct test and example files with direct Trx reception #244 tested and merged
- you can remove these -15dBm@transponder_RX change from this PR

Which one is better from your point of view?

I apologize that #244 is not in yet, but I got stuck at #249 (help from anyone is appreciated there, I simply do not understand the EDFA model enough to make progress).

EstherLerouzic · 2019-05-31T10:53:08Z

OK ! thanks for the feedback !
I prefer to do the backporting this series on top of recent develop
and I will remove the -15dBm@transponder_RX change from this PR

jktjkt · 2019-05-31T13:48:39Z

Thanks for completing this. Now the git history of this PR contains stuff which undoes another stuff done within this pull request, and I do not think that it's worth it to preserve such a level of detail. The new feature in general is reasonably compact, so it can be merged in a single commit (or two, with the utils/ change separated out). I'll force-push a cleanup shortly.

jktjkt · 2019-05-31T14:04:23Z

gnpy/core/equipment.py

@@ -134,7 +138,8 @@ def from_json(cls, filename, **kwargs):
        config = Path(filename).parent / 'default_edfa_config.json'

        type_variety = kwargs['type_variety']
-        type_def = kwargs.get('type_def', 'variable_gain') #default compatibility with older json eqpt files
+        type_def = kwargs.get('type_def', 'variable_gain')
+        #default compatibility with older json eqpt files


I think that it's more intuitive if these comments are listed at the same line, or before the line they refer to. I've cleaned this.

gnpy/core/network.py

jktjkt · 2019-05-31T14:20:09Z

@EstherLerouzic , can you please check that the descriptions given in the commit at the tip of this branch and in the README reflect reality? I tried to add a summary of what this thing is about into the commit message, and I also updated the README part about ROADMs.

EstherLerouzic · 2019-06-03T10:27:26Z

@jktjkt, I think the sentence "In case a per-degree exception is needed, add the Fused node to a
specific degree. When the Fused element is present, the restrictions no longer apply, and amplifiers can be placed manually."
should rather be something like:
"If a per-degree exception is needed, the amplifier of this degree can be defined in the Eqpt sheet or in the network json input. If no booster amplifier should be placed on a degree:

(in case of an excel input) the term fused should be used for the amplifier type in the eqpt sheet. This will disable the automatic placement of an amplifier on this degree by setting a 'Fused' (logical) node instead.
(In case of a json input), simply insert a 'Fused' node on the Roadm degree output.
"
Thanks for the changes !

jktjkt · 2019-06-03T10:45:18Z

@EstherLerouzic , could you please check the updated wording? I've verified that you already put a correct wording into the Excel documentation, so I adjusted the commit message and clarified the JSON format in the main README.rst.

EstherLerouzic · 2019-06-03T11:59:06Z

@jktjkt , the new wording is not correct for the fused part with excel input
'If no booster amplifier should be placed on a degree, use the Fused
node in place of an amplifier.'
because there will be an edfa placed between the ROADM and the 'Fused' node. This is why I have introduced the 'fused' (small 'f' ) keyword in the Eqpt parsing.
the correct sentence should be something like :
"If no booster amplifier should be placed on a degree, use a Fused
node in place of an amplifier in case of json input, or use the 'fused' keyword for the amplifier type in the eqpt sheet for excel input. This will disable the automatic placement of an amplifier on this degree."

The Change in README is OK for me

jktjkt

I apologize that I missed this during my review -- apparently I got confused by Fused vs. fused. I like that you described this as a hack, but it did not trip me for some reason. Sorry for that.

I do not like this approach. It sounds like a workaround for a deficiency in the Excel->JSON conversion. Rather than creating Edfa nodes with type_variety fused and replacing them here in gnpy/core/network.py, let's fix the Excel parser so that it creates correct data. That way we won't have to maintain extra (and undocumented) feature in a place where it does not belong (IMHO).

Right now I do not see this feature being used in the example Excel files (or am I wrong?). The test is there, but it modifies an input so that it "matches" how the Excel->JSON conversion output would look like.

I would prefer to:

Have some Excel file modified with this lowercase-fused no-booster use case. This should be done by you to prevent future errors on my side.
Move the parsing code into convert.py and adjust the unit tests. I can do this, or you can do this -- whatever you prefer.

EstherLerouzic · 2019-06-03T13:57:52Z

I have done the requested changes.

tests/test_roadm_restrictions.py

EstherLerouzic · 2019-06-05T07:30:22Z

@jktjkt ,
in my last commits I corrected a bug (sorry) + updated tests and rebased all this on the latest develop . Could you check if this is OK for you ?
Besides, all tests are running ok on my computer, but the PR is not passing tests via travis on github: any idea ? (I have found no detailed report on the cause of this failure, and don't know what to correct). Thanks a lot for your help !

EstherLerouzic · 2019-06-05T09:58:56Z

I found the issue !
docstring was not correct with the bug correction, and since it is tested with travis, the test fails

Co-authored-by: Esther Le Rouzic <esther.lerouzic@orange.com>

This feature is intended to support designs such as OpenROADM where the line degree integrates a specific preamp/booster pair. In that case, it does not make sense for our autodesign to "pick an amplifier". The restrictions can be activated by: - Listing them in `eqpt_config.json`, so that they are effective for all ROADM instances. - On a per-ROADM basis within the Excel sheet or the JSON definitions. Restrictions apply to an entire ROADM as a whole, not to the individual degrees. If a per-degree exception is needed, the amplifier of this degree can be defined in the equipment sheet or in the network definition. If no booster amplifier should be placed on a degree, use the `Fused` node in place of an amplifier. Signed-off-by: Esther Le Rouzic <esther.lerouzic@orange.com> Co-authored-by: Jan Kundrát <jan.kundrat@telecominfraproject.com>

jktjkt

I've squashed them into just two commits, and made some minor clean-ups, e.g. this:

if not key in foo:
  raise Foo(...)

instead of trying to access foo[key] and catching the KeyError exception.

I'll merge this once all check are completed.

jktjkt reviewed May 17, 2019

View reviewed changes

jktjkt added this to WIP in GNPy May 21, 2019

EstherLerouzic force-pushed the build_no_amp_in_roadm branch from 6e7a2a7 to 6010301 Compare May 21, 2019 16:36

EstherLerouzic marked this pull request as ready for review May 21, 2019 16:37

This comment has been minimized.

Sign in to view

EstherLerouzic force-pushed the build_no_amp_in_roadm branch 2 times, most recently from 263acad to 8d45f98 Compare May 23, 2019 12:31

jktjkt requested changes May 23, 2019

View reviewed changes

jktjkt added a commit to jktjkt/oopt-gnpy that referenced this pull request May 23, 2019

tests: consult the doctests as well

cbb61f1

There are no doctests right now, but they will (might?) be added (see PR Telecominfraproject#230).

EstherLerouzic force-pushed the build_no_amp_in_roadm branch from 3a795ec to 038fe18 Compare May 24, 2019 13:39

jktjkt reviewed May 27, 2019

View reviewed changes

EstherLerouzic force-pushed the build_no_amp_in_roadm branch from 038fe18 to cc14496 Compare May 31, 2019 11:39

jktjkt reviewed May 31, 2019

View reviewed changes

gnpy/core/network.py Outdated Show resolved Hide resolved

jktjkt force-pushed the build_no_amp_in_roadm branch 2 times, most recently from ff22e5f to 69f398d Compare May 31, 2019 14:12

jktjkt assigned EstherLerouzic May 31, 2019

jktjkt force-pushed the build_no_amp_in_roadm branch from 69f398d to 985de51 Compare May 31, 2019 14:19

jktjkt moved this from Needs Coding to Needs Review in GNPy May 31, 2019

jktjkt previously approved these changes May 31, 2019

View reviewed changes

jktjkt dismissed their stale review via d2df792 June 3, 2019 10:42

jktjkt force-pushed the build_no_amp_in_roadm branch from 985de51 to d2df792 Compare June 3, 2019 10:42

GNPy automation moved this from Needs Review to Needs Coding Jun 3, 2019

jktjkt requested changes Jun 3, 2019

View reviewed changes

tests/test_roadm_restrictions.py Show resolved Hide resolved

EstherLerouzic force-pushed the build_no_amp_in_roadm branch from fcfedbf to e94a78e Compare June 4, 2019 16:50

Utility functions for pruning and merging

22acd88

Co-authored-by: Esther Le Rouzic <esther.lerouzic@orange.com>

jktjkt force-pushed the build_no_amp_in_roadm branch from 0e1fc2f to b07efa4 Compare June 6, 2019 09:43

jktjkt force-pushed the build_no_amp_in_roadm branch from b07efa4 to d94dc51 Compare June 6, 2019 09:59

jktjkt approved these changes Jun 6, 2019

View reviewed changes

jktjkt merged commit 5be30d8 into Telecominfraproject:develop Jun 6, 2019

GNPy automation moved this from Needs Coding to Done Jun 6, 2019

EstherLerouzic deleted the build_no_amp_in_roadm branch September 23, 2019 15:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build no amp in roadm and add roadm restrictions on amps #230

Build no amp in roadm and add roadm restrictions on amps #230

EstherLerouzic commented May 17, 2019

jktjkt left a comment

This comment has been minimized.

EstherLerouzic commented May 23, 2019

jktjkt May 23, 2019

EstherLerouzic May 24, 2019

jktjkt May 24, 2019

jktjkt commented May 24, 2019

jktjkt May 27, 2019

EstherLerouzic commented May 31, 2019

jktjkt commented May 31, 2019

EstherLerouzic commented May 31, 2019

jktjkt commented May 31, 2019

jktjkt May 31, 2019

jktjkt commented May 31, 2019

EstherLerouzic commented Jun 3, 2019

jktjkt commented Jun 3, 2019

EstherLerouzic commented Jun 3, 2019

jktjkt left a comment

EstherLerouzic commented Jun 3, 2019

EstherLerouzic commented Jun 5, 2019

EstherLerouzic commented Jun 5, 2019

jktjkt left a comment

Build no amp in roadm and add roadm restrictions on amps #230

Build no amp in roadm and add roadm restrictions on amps #230

Conversation

EstherLerouzic commented May 17, 2019

jktjkt left a comment

Choose a reason for hiding this comment

This comment has been minimized.

EstherLerouzic commented May 23, 2019

jktjkt May 23, 2019

Choose a reason for hiding this comment

EstherLerouzic May 24, 2019

Choose a reason for hiding this comment

jktjkt May 24, 2019

Choose a reason for hiding this comment

jktjkt commented May 24, 2019

jktjkt May 27, 2019

Choose a reason for hiding this comment

EstherLerouzic commented May 31, 2019

jktjkt commented May 31, 2019

EstherLerouzic commented May 31, 2019

jktjkt commented May 31, 2019

jktjkt May 31, 2019

Choose a reason for hiding this comment

jktjkt commented May 31, 2019

EstherLerouzic commented Jun 3, 2019

jktjkt commented Jun 3, 2019

EstherLerouzic commented Jun 3, 2019

jktjkt left a comment

Choose a reason for hiding this comment

EstherLerouzic commented Jun 3, 2019

EstherLerouzic commented Jun 5, 2019

EstherLerouzic commented Jun 5, 2019

jktjkt left a comment

Choose a reason for hiding this comment