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

Network set interpolate possible fix for Issue 999 #1012

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Sionwage
Copy link

@Sionwage Sionwage commented Jan 26, 2024

As outlined in Issue #999, skrf.NetworkSet.interpolate_from_params raises exception if any identical parameter values are in a network set where as skrf.NetworkSet.interpolate_from_network using the same parameters does not.

Root cause is because sub_ns.coords[param] is called which omits duplicate parameter values while passing the duplicate networks by the sub_ns = self.sel(sub_params)

Changed the interp_kind to be both str and int to match the scipy.interp1d function and added the warning message about duplicate parameters may cause the function to fail depending on the interpolation kind.

Would it be desired to add a flag to average duplicate networks or omit them to prevent the scipy.interp1d from failing?

Sionwage added 3 commits January 26, 2024 17:07
…ters used matches the number of networks selected. Also changed NetworkSet.interpolate_from_network and NetworkSet.interpolate_from_params comments to indicate interp_kind can also take a number and duplicate parameter values may cause the scipy.interp1d function to fail.
@@ -1305,7 +1308,7 @@ def interpolate_from_params(self, param: str, x: float,

# interpolating the sub-NetworkSet matching the passed sub-parameters
sub_ns = self.sel(sub_params)
interp_ntwk = sub_ns.interpolate_from_network(sub_ns.coords[param],
interp_ntwk = sub_ns.interpolate_from_network(sub_ns.params_values[param],
Copy link
Member

Choose a reason for hiding this comment

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

this will not work for unordered parameters (which explains why tests are failing)

Copy link
Author

Choose a reason for hiding this comment

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

I am unfamiliar with setting up a development environment for testing besides using poetry. Any suggestions for initializing a development setup from the pyproject.toml?

I have created a fresh conda enviroment and manually installed the listed modules.

After running pip install . from the root folder, I was finally able to get the unittests to run.

Pre-commit on the other hand is a mess. Conda seems to throw the following error.

(skrf-git) PS C:\Users\user\OneDrive\Documents\Python Git\scikit-rf> pre-commit install                                                                                                      pre-commit installed at .git\hooks\pre-commit
(skrf-git) PS C:\Users\user\OneDrive\Documents\Python Git\scikit-rf> git commit -m "Added comment to line to test pre-commit"                                                                An error has occurred: InvalidConfigError:
==> File .pre-commit-config.yaml
==> At Config()
==> At key: exclude
=====> '(?x)(\n     ^conda.recipe/meta.yaml|\n     (?i).*\\.(csv|s[0-9]+p|mdf|cti|ts|svg)\n)\n' is not a valid python regex
Check the log at C:\Users\user\.cache\pre-commit\pre-commit.log
(skrf-git) PS C:\Users\user\OneDrive\Documents\Python Git\scikit-rf>

version information

pre-commit version: 3.6.0
git --version: git version 2.41.0.windows.3
sys.version:
    3.12.1 | packaged by Anaconda, Inc. | (main, Jan 19 2024, 15:44:08) [MSC v.1916 64 bit (AMD64)]
sys.executable: C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\python.exe
os.name: nt
sys.platform: win32

error information

An error has occurred: InvalidConfigError: 
==> File .pre-commit-config.yaml
==> At Config()
==> At key: exclude
=====> '(?x)(\n     ^conda.recipe/meta.yaml|\n     (?i).*\\.(csv|s[0-9]+p|mdf|cti|ts|svg)\n)\n' is not a valid python regex
Traceback (most recent call last):
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\site-packages\pre_commit\error_handler.py", line 73, in error_handler
    yield
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\site-packages\pre_commit\main.py", line 386, in main
    return hook_impl(
           ^^^^^^^^^^
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\site-packages\pre_commit\commands\hook_impl.py", line 271, in hook_impl
    return retv | run(config, store, ns)
                  ^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\site-packages\pre_commit\commands\run.py", line 422, in run
    config = load_config(config_file)
             ^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\site-packages\cfgv.py", line 399, in load_from_filename
    with reraise_as(exc_tp):
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\contextlib.py", line 158, in __exit__
    self.gen.throw(value)
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\site-packages\cfgv.py", line 45, in reraise_as
    raise tp(e).with_traceback(tb) from None
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\site-packages\cfgv.py", line 42, in reraise_as
    yield
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\site-packages\cfgv.py", line 403, in load_from_filename
    with validate_context(f'File {display_filename}'):
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\contextlib.py", line 158, in __exit__
    self.gen.throw(value)
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\site-packages\cfgv.py", line 36, in validate_context
    raise ValidationError(e, ctx=msg).with_traceback(tb) from None
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\site-packages\cfgv.py", line 33, in validate_context
    yield
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\site-packages\cfgv.py", line 415, in load_from_filename
    validate(data, schema)
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\site-packages\cfgv.py", line 378, in validate
    schema.check(v)
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\site-packages\cfgv.py", line 244, in check
    with validate_context(context):
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\contextlib.py", line 158, in __exit__
    self.gen.throw(value)
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\site-packages\cfgv.py", line 36, in validate_context
    raise ValidationError(e, ctx=msg).with_traceback(tb) from None
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\site-packages\cfgv.py", line 33, in validate_context
    yield
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\site-packages\cfgv.py", line 246, in check
    item.check(v)
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\site-packages\cfgv.py", line 55, in _check_optional
    with validate_context(f'At key: {self.key}'):
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\contextlib.py", line 158, in __exit__
    self.gen.throw(value)
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\site-packages\cfgv.py", line 36, in validate_context
    raise ValidationError(e, ctx=msg).with_traceback(tb) from None
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\site-packages\cfgv.py", line 33, in validate_context
    yield
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\site-packages\cfgv.py", line 56, in _check_optional
    self.check_fn(dct[self.key])
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\site-packages\cfgv.py", line 373, in check
    fn(v)
  File "C:\Users\user\AppData\Local\anaconda3\envs\skrf-git\Lib\site-packages\cfgv.py", line 354, in check_regex
    raise ValidationError(f'{v!r} is not a valid python regex')
pre_commit.clientlib.InvalidConfigError: 
==> File .pre-commit-config.yaml
==> At Config()
==> At key: exclude
=====> '(?x)(\n     ^conda.recipe/meta.yaml|\n     (?i).*\\.(csv|s[0-9]+p|mdf|cti|ts|svg)\n)\n' is not a valid python regex

Copy link
Author

Choose a reason for hiding this comment

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

I got a working environment now after #1018 and setting up conda develop, I'll try to change my pull request to pass the tests.

Copy link
Author

Choose a reason for hiding this comment

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

The pertinent test code:

setUp(self)

self.freq1 = rf.Frequency(75, 110, 101, 'ghz')

self.params = [
                {'a':0, 'X':10, 'c':'A'},
                {'a':1, 'X':10, 'c':'A'},
                {'a':2, 'X':10, 'c':'A'},
                {'a':1, 'X':20, 'c':'A'},
                {'a':0, 'X':20, 'c':'A'},
            ]

self.ntwks_params = [rf.Network(frequency=self.freq1,
                                s=np.random.rand(len(self.freq1),2,2),
                                name=f'ntwk_{m}',
                                comment=f'ntwk_{m}',
                                params=params) \
                     for (m, params) in enumerate(self.params) ]

self.ns_params = rf.NetworkSet(self.ntwks_params)

Failing test

# ambiguity: could interpolate a for X=10 or X=20...
self.assertRaises(ValueError, self.ns_params.interpolate_from_params, 'a', 0.5)

I don't think the unordered parameter set is the issue but the ambiguity of not throwing a value error as the interpolation could be with conflicting X=10 or X=20 values.

To have the same behavior as before, I need to add a function to check that all unspecified parameters are the same value for the entire network set being interpolated or else raise a ValueError noting other parameters are changing as well.

The issue I had and was hoping to correct was if there was a duplicate measurements in the network set that interpolate_from_params would still work if the self.params was instead this value:

self.params = [
                {'a':0, 'X':10, 'c':'A'},
                {'a':1, 'X':10, 'c':'A'},
                {'a':1, 'X':10, 'c':'A'},
                {'a':2, 'X':10, 'c':'A'},
                {'a':1, 'X':10, 'c':'A'},
                {'a':1, 'X':20, 'c':'A'},
                {'a':0, 'X':20, 'c':'A'},
            ]

I know that since interpolate_from_params defaults to linear, the duplicate 'a' values of 1 for 'X'=10 would still work even with them being out of order with the scipy.interp1d function and the NetworkSet.sel function would keep the parameters ordered the same as the networks.

Some of the scipy.interp1d interpolation methods will take duplicate input values and some will not. I was thinking that using the mean_s method from NetworkSet to reduce the NetworkSet to only unique values for a parameter to be interpolated could be of some value.

I'll fix my PR so the unittests pass and so interpolate_from_params and interpolate_from_network act the same for the same networks and params used. I would like to try and propose a separate PR later to either average duplicated parameters as a subNetworkSet or omit networks after the first parameter instance is used to all both functions to not fail if interpolation kinds other than linear are used with scipy.interp1d unless that is undesired behavior.

Also, as you mentioned, I believe the function if given unsorted networks for the parameters would sort the parameter values but leave the selected subNetworkSet out of order before interpolating. NetworkSet.sel doesn't look like it sorts but NetworkSet.coords looks like it does for a given parameter.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for your detailed answer. I undertand the issue.

Some of the scipy.interp1d interpolation methods will take duplicate input values and some will not. I was thinking that using the mean_s method from NetworkSet to reduce the NetworkSet to only unique values for a parameter to be interpolated could be of some value.

Taking the mean value in case of a set of all identical parameters is indeed a possible solution. (But one think of using other operator, like median, or more fancy statistics?)

I'll fix my PR so the unittests pass and so interpolate_from_params and interpolate_from_network act the same for the same networks and params used. I would like to try and propose a separate PR later to either average duplicated parameters as a subNetworkSet or omit networks after the first parameter instance is used to all both functions to not fail if interpolation kinds other than linear are used with scipy.interp1d unless that is undesired behavior.

Yes, this could be two different behaviour: skip the other Networks in the set having the same parameters or making a mean or something else...

For example in pandas, the duplicated method allows the option keep='first' or 'last'. To make it more general and allowing different use cases, we should maybe think about a kind of .groupby('parameter') method for a NetworkSet ? Like in pandas, so one could cascade the methods as in groupby('parameter').mean() ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants