Skip to content

SRS tests - #132

Open
manny118 wants to merge 2 commits into
masterfrom
SRS_tests2
Open

SRS tests#132
manny118 wants to merge 2 commits into
masterfrom
SRS_tests2

Conversation

@manny118

@manny118 manny118 commented Aug 5, 2022

Copy link
Copy Markdown
Contributor

Tests for the SRS models.

@manny118 manny118 mentioned this pull request Aug 5, 2022
@manny118

manny118 commented Aug 5, 2022

Copy link
Copy Markdown
Contributor Author

Extends the previous request here.

Comment thread mnoptical/link.py
from pprint import pprint
from numpy import errstate
from mnoptical.node import LineTerminal, Roadm, Amplifier
from mnoptical.edfa_params import fibre_spectral_attenuation

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All tests are failing because the fibre_spectral_attenuation list is not in edfa_params.py in this PR.
Might be worth adding it to see if tests are passing

@lantz lantz Aug 8, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that should have been included in this PR instead. However we can merge #133 and see if it helps.

@lantz

lantz commented Aug 8, 2022

Copy link
Copy Markdown
Collaborator

@juraul1 @manny118 I enabled the (somewhat recently added) repo setting enabling admins to update (i.e. git rebase) pull requests. For an out-of-date branch that merges cleanly, there should now be an update button (visible to admins at least.)

It's a very useful feature, and I just used it to update/rebase this PR after merging #133.

@lantz

lantz commented Aug 8, 2022

Copy link
Copy Markdown
Collaborator

@manny118 Now the SRS tests look like they are legitimately failing:

======================================================================
FAIL: test_SRS (__main__.TestSRS)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/mininet-optical/mininet-optical/tests/SRS_Test2.py", line 124, in test_SRS
    self.assertEqual(actual_result, expected_result)
AssertionError: 0.62 != 1.01
----------------------------------------------------------------------

see line 744 of: https://github.com/Mininet-Optical/mininet-optical/runs/7729810659?check_suite_focus=true#step:7:744

So, the SRS tests are doing their thing, which is good. Now we just have to get them passing.

@manny118

manny118 commented Aug 8, 2022

Copy link
Copy Markdown
Contributor Author

The first script passed whereas others failed, and that is because each script calls a different SRS function. Only one function can be enabled each time in the code.

@lantz

lantz commented Aug 8, 2022

Copy link
Copy Markdown
Collaborator

Could you fix it so that all SRS functions are tested and the tests pass?

We can't merge the branch unless the tests pass.

@manny118

manny118 commented Aug 8, 2022

Copy link
Copy Markdown
Contributor Author

An option would be to use 4 different pull requests, although the same link.py script would be used for each. I am not sure of how that would work since it would mean having all four functions enabled in the code when they are merged into the repo which is not ideal.

@lantz

lantz commented Aug 8, 2022

Copy link
Copy Markdown
Collaborator
  1. Shouldn't srs_effect=True be the default?
  2. Could you explicitly add srs_effect=True as a Link parameter in your test code?
  3. How is the SRS model specified? Could you do something like this in link.py?:
...
class Link:
    srs_models = [MyCoolSRSModel, SomeOtherSRSModel, ..., None]
    srs_model = MyCoolSRSModel
    def __init__(self, ... **params):
        ...
       self.srs_model = params.get('srs_model', self.srs_model)

...
    def propagate(....):
        if self.srs_model: 
            self.srs_model(...)
        

That would also replace srs_effect=False with srs_model=None which makes sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants