Change repeated capabilities #718

texasaggie97-zz · 2018-01-04T21:53:45Z

[X] This contribution adheres to CONTRIBUTING.md.
[X] I've updated CHANGELOG.md if applicable.
[X] I've added tests applicable for this pull request

What does this Pull Request accomplish?

Moves repeated capabilities away from session object and on to specific instances of the different types
Repeated capability can be integer or string, single item or list

List issues fixed by this Pull Request below, if any.

What testing has been done?

System
Unit

auchter · 2018-01-04T22:05:09Z

build/templates/session.py.mako

+    def __getitem__(self, repeated_capability):
+        '''Set/get properties or call methods with a repeated capability (i.e. channels)'''
+        if isinstance(repeated_capability, list):
+            rep_cap_list = [str(r) if str(r).startswith('${rep_cap['prefix']}') else '${rep_cap['prefix']}' + str(r) for r in repeated_capability]


Hmmm... IVI repeated capability strings are case insensitive, but the prefix check here is case sensitive.

Made check case insensitive.

marcoskirsch · 2018-01-08T18:45:23Z

CHANGELOG.md

+        session = niscope.Session('PXI1Slot2')
+
+        # Channel repeated capabilities
+        session.channel['0'].channel_enabled = True


since this is a container that you're indexing, shouldn't it be in plural form ("channels")?

Changed to channels

marcoskirsch · 2018-01-08T18:45:53Z

CHANGELOG.md

+        session.channel[[0, 1, 3]].channel_enabled = True
+        wfm = session.channel[[0, 1, 3]].fetch(5000)
+
+        # P2P repeated capabilities


P2P is not supported, right? Maybe use NI-FGEN MarkerEvents for the example?

I think we should add P2P attributes back.

marcoskirsch · 2018-01-08T18:46:28Z

CHANGELOG.md

  * #### Changed
  * #### Removed
 * ### NI-FGEN
  * #### Added
+    * Channel repeated capability
+    * P2P repeated capability


I thought we took all P2P stuff out of the Python bindings.

marcoskirsch · 2018-01-08T18:46:56Z

CHANGELOG.md

@@ -29,14 +49,19 @@ All notable changes to this project will be documented in this file.
  * #### Removed
 * ### NI-DCPower
  * #### Added
+    * Channel repeated capability
  * #### Changed
  * #### Removed
 * ### NI-FGEN
  * #### Added


Script Triggers, Marker Events are two NI-FGEN and NI-HSDIO repeated capabilities. Don't know of others.

marcoskirsch · 2018-01-08T18:48:14Z

build/helper/documentation_helper.py

@@ -402,7 +402,7 @@ def _format_type_for_docstring(param, numpy, config):


 rep_cap_method_desc_docstring = rep_cap_method_desc + '''
-    session['0,1'].{1}({2})
+    session.channel[[0, 1]].{1}({2})


Question for the Python crowd... is indexing using an array "Pythonic"? Or is it strange?

Definitely weird to me.
Looking at the documentation of __getitem__ it says "For sequence types, the accepted keys should be integers and slice objects.".
So here the user should be able to say session.channel[0:1] (since 0:1 represents a slice).

We'd have to give it more thought if you intend to support the user passing in channels that don't fall under the umbrella of "integer or slice" (I.e. channels 0, 1, 4, and 12). But depending on what __getitem__ returns you could try and support something like my_channels = session.channels[0:1] + session.channels[4] + session.channels[12].

Channels 0, 1, 4, 12 definitely needs to be supported. Something like my_channels = session.channels[0:1] + session.channels[4] + session.channels[12] to do it though seems like a step backwards.

If you wanted a list that contains elements at indices 0, 1, 4, and 12, how would you construct that list?

Indexing using arrays is not strange at all. See Numpy's advanced indexing feature: https://docs.scipy.org/doc/numpy-1.13.0/reference/arrays.indexing.html#advanced-indexing

After seeing numpy's "advanced-indexing" I think allowing an array makes sense. I think as long as the user can use integers and slices then users will find themselves successful. Allowing arrays seems like a "nice-to-have" but if it is easy to support, and the user can easily understand what happens when an array is used, then I say go for it.

We need one object where we can internally put together a string that looks like '0,1,4,12'.

The string is an implementation detail. What you want is for the user to be able to say "on channels x do y". Since we're exposing the "seuqence" version of __getitem__ on the channels attribute to access specific channels, I see it the same as accessing indices of a sequence type to operate on.

Yes, it is an implementation detail, but it is an important detail. We need the call to condense down to a single call with '0,1,4,12' not a call with '0,1', then a call with '4', then a call with '12'.

It is very possible that I don't see something that would allow my_channels = session.channels[0:1] + session.channels[4] + session.channels[12] to do this. But also, this seems very verbose.

(session.channels[0:1] + session.channels[4] + session.channels[12]).fetch(1000) seems unwieldy.

This can probably be achieved, if we decide that's the way we want to go, by implementing add().

The string is an implementation detail.

Under the hood, this will touch hardware. If a user sets the voltage on several channels "all at once" but the Python code somewhat serializes this, DUT can be damaged. Mark is right that we need to make this work.

I agree serializing the calls is not an option. I just want to make sure the implementation details don't bleed out into the interface. After seeing that numpy uses arrays to mean a list of indices, this is starting to look less weird.

I also don't think add() should be added to the type that __getitem__() returns just yet (mainly because the list-of-indices should cover most of the use cases). I just wanted to bring it up to show the similarities between two sequence types (yours and lists), to help us think about how we should design the interface,

Just make sure the normal use case isn't broken (integers and slices), and the array-of-indices has good documentation and I think you're good to go. 👍

marcoskirsch · 2018-01-08T18:51:03Z

build/templates/session.py.mako

+        else:
+            rep_cap_list = [str(repeated_capability) if str(repeated_capability).lower().startswith('${rep_cap['prefix'].lower()}') else '${rep_cap['prefix']}' + str(repeated_capability)]
+
+        return _RepeatedCapbilities(${config['session_handle_parameter_name']}=self._${config['session_handle_parameter_name']}, repeated_capability=','.join(rep_cap_list), library=self._library, encoding=self._encoding, freeze_it=True)


_RepeatedCapbilities

typo

marcoskirsch · 2018-01-08T18:54:31Z

generated/nidcpower/session.py

+        return _RepeatedCapbilities(vi=self._vi, repeated_capability=','.join(rep_cap_list), library=self._library, encoding=self._encoding, freeze_it=True)
+
+
+class _RepeatedCapbilities(object):
    '''Base class for all NI-DCPower sessions.'''


Seems odd that a class named _RepeatedCapbilities is 'Base class for all NI-DCPower sessions.'

What should it be called? It is the class that contains any attribute (all) or function that takes a repeated capabilities string.

I haven't come up with a perfect name for it but _SessionBase is probably better than _RepeatedCapabilities.

When I read _RepeatedCapabilities, I think of a collection of repeated capabilities such as "0" or "ScriptTrigger2".

marcoskirsch · 2018-01-08T19:03:20Z

build/templates/session.py.mako

+
+    def __getitem__(self, repeated_capability):
+        '''Set/get properties or call methods with a repeated capability (i.e. channels)'''
+        if isinstance(repeated_capability, list):


This seems restrictive. Clients should be able to pass anything iterable even if not a list. For example, range() doesn't return a list.

Changed to use duck typing. Anything iterable will now work.

Comments addresses

texasaggie97-zz · 2018-01-09T17:42:30Z

There is a scope system test failing because it sets test_channels = '0,1'. Should I

Fix the test to say test_channels = range(2)
Fix the code to allow it?

All comments addressed

…tems we need

marcoskirsch · 2018-02-07T16:38:18Z

CHANGELOG.md

+    * #### Added
+        * Repeated capablilites - See #737 for discussion:
+            * `channel` repeated capability
+            * `p2p_streams` repeated capability


Since #724 is not merged yet, nor is it decided upon, you should remove p2p_streams from this PR.

marcoskirsch · 2018-02-07T16:39:19Z

README.rst

+    .. code-block:: python
+
+        import nifgen
+        session = nifgen.Session('PXI1Slot2')


how come you didn't use with? If you'd still rather not, then maybe show how you close the session?

I wasn't trying to show how to write an actual useful program. Added session.close()

marcoskirsch · 2018-02-07T16:39:50Z

README.rst

+        session.channels[:8].channel_enabled = True  # channels 0, 1, 2, 3, 4, 5, 6, 7
+        wfm = session.channels[[0, 1, 3]].fetch(5000)
+
+        # P2P repeated capabilities


bad comment

marcoskirsch · 2018-02-07T16:40:21Z

README.rst

+        wfm = session.channels[[0, 1, 3]].fetch(5000)
+
+        # P2P repeated capabilities
+        i = session.script_triggers['0'].SCRIPT_TRIGGERS_COUNT


use python_names for the attributes.

marcoskirsch · 2018-02-07T16:41:19Z

README.rst

+        i = session.script_triggers['0'].SCRIPT_TRIGGERS_COUNT
+        i = session.script_triggers[0].SCRIPT_TRIGGERS_COUNT
+        i = session.script_triggers[[0, 1, 3]].SCRIPT_TRIGGERS_COUNT
+        i = session.script_triggers['ScriptTrigger0'].SCRIPT_TRIGGERS_COUNT


Does this actually work? you don't get "ScriptTriggerScriptTrigger0" passed into the C function?
If it works, I'd consider it an implementation fluke and would not document it.

Yes, this works and it works by design. I explicitly look for the prefix and only add if it is not there.

Do you think we should never accept strings that already have the prefix?

Do you think we should never accept strings that already have the prefix?

Well, the whole point of these containers is for the client to not have to remember magic strings. I don't think it needs to be documented, even if we still accept them.

Okay, removed from readme.

marcoskirsch · 2018-02-07T16:43:23Z

build/helper/parameter_usage_options.py

@@ -5,32 +5,36 @@ class ParameterUsageOptions(Enum):
    '''Different usage options for parameter lists.'''

    SESSION_METHOD_DECLARATION = 1


We should just use auto here.

auto() is only available in 3.6 and the enum backport. I used the solution given in the 3.5 documentation:

class AutoNumber(Enum): def __new__(cls): value = len(cls.__members__) + 1 obj = object.__new__(cls) obj._value_ = value return obj class ParameterUsageOptions(AutoNumber): '''Different usage options for parameter lists.''' SESSION_METHOD_DECLARATION = () '''For declaring a regular method in Session''' SESSION_INIT_DECLARATION = ()```

marcoskirsch · 2018-02-07T16:45:36Z

build/templates/_converters.py.mako

@@ -7,6 +7,26 @@ from ${module_name} import visatype
 import datetime


+def convert_repeated_capabilities(repeated_capability, prefix=''):
+    # First try it as a list
+    if isinstance(repeated_capability, list):


This might be too restrictive. What if the structure is something other than a list, like array.array? Python user would expect it to work because duck typing.
Consider implementing with try/except pattern instead of looking for specific type.

I did that at first. This is fixing the issue that broke using string. Strings are iterable so it was iterating over the string and that is what would turn '0,1' into '0,,,1' and 'ScriptTrigger0' into 'S,c,r,i,p,t,T,r,i,g,g,e,r,0'.

I would need to explicitly look for a string and handle that first, and then try to iterate.

I think that's reasonable.

Changed the logic to first look for six.string_type or six.text_type. This allows us to accept any iterable.

This adds a dependency to six for all drivers.

marcoskirsch · 2018-02-07T16:46:13Z

build/templates/_converters.py.mako

+        try:
+            def ifnone(a, b):
+                return b if a is None else a
+            # Turn the slice into a list so we can iterate over it


If you turn the slice into a list, can't you just call this function with the list recursively?

marcoskirsch · 2018-02-07T16:50:38Z

generated/nifake/session.py

@@ -113,10 +140,16 @@ class _SessionBase(object):
    An attribute of type string with read/write access.
    '''

-    def __init__(self, repeated_capability):
-        self._library = library_singleton.get()
+    def __init__(self, repeated_capability, vi=None, library=None, encoding=None, freeze_it=False):


I think it's less error prone if they weren't optional and Session passes them when it chains up to its parent class' __init__() method.

All comments addressed

Adds dependency on six to the drivers

texasaggie97-zz added 5 commits January 4, 2018 15:39

Add repeated_capabilities key in config for all drivers

3dc54c9

Update tests for new method of accessing repeated capabilities

ad95cba

Change how repeated capabilities are handled

10eade5

Update generated files

3860ec6

Update changelog

a89cd1f

auchter reviewed Jan 4, 2018

View reviewed changes

texasaggie97-zz added 8 commits January 4, 2018 16:07

Add function example in changelog

bf49be6

Case insensitive check

ea96d19

Missed a channel reference

8458488

Update generated files

b6c1330

Fix parent/child instance variables

0467916

Update generated files

339908b

Fix up channel based tip admonition for new syntax

8ba8760

Update generated files

fe0b7a1

marcoskirsch previously requested changes Jan 8, 2018

View reviewed changes

texasaggie97-zz added 9 commits January 8, 2018 14:48

Change channel rep cap class to plural - channels

a5ee751

Add Marker and Script Trigger repeated capabilities to FGen

5e51689

Fix typo in names

cb97f7f

Update tests for plural channels

186dd6c

Use duck typing insted of checking for list

e4152b9

Fix tests

c764db6

Update generated files

86c220a

Fix tests to use range() instead of driver channel list format

5790fb3

Add example to show using range() as a rep cap list

2832f66

texasaggie97-zz added 2 commits January 8, 2018 17:23

Fix range usage

ae065a2

Fix range usage

6588ecf

texasaggie97-zz added 2 commits January 10, 2018 12:12

Merge branch 'master' into api/rep_caps

798ba75

Merge branch 'master' into api/rep_caps

e5c2322

texasaggie97-zz added 7 commits February 5, 2018 14:49

Test int and string

6eab9b4

Remove now redundant tests

7337b8e

Update generated files

24b67f7

Only define one class and instantiate multiple times

d08ea33

Spaces matter

ff3dfb3

Update generated files

603e9ac

Add comments in __init__

61c7d2f

texasaggie97-zz added 2 commits February 5, 2018 17:17

Initialize _RepeatedCapabilities with Session instead of individual i…

0b43ad4

…tems we need

Update generated files

e9891b7

marcoskirsch previously requested changes Feb 7, 2018

View reviewed changes

marcoskirsch changed the title ~~[2]Change repeated capabilities~~ Change repeated capabilities Feb 7, 2018

texasaggie97-zz added 6 commits February 7, 2018 17:48

Remove p2p mentions.

ff9213b

Use auto numbering for the enum

fe9e2c0

Fix readme

ed34c39

Once we have a list, we just call ourselves with it

7fa18ce

No default parameters on _SessionBase __init__, pass None in explicitly

2026eb0

Update generated files

d7ae7d6

texasaggie97-zz added 3 commits February 8, 2018 12:30

Change logic for getting repeated capability string

951f5f9

Adds dependency on six to the drivers

Don't show using prefix

d7f89d2

Update generated files

b2c8bbb

marcoskirsch approved these changes Feb 8, 2018

View reviewed changes

marcoskirsch merged commit 94141c3 into master Feb 8, 2018

marcoskirsch deleted the api/rep_caps branch February 8, 2018 19:56

marcoskirsch mentioned this pull request Feb 14, 2018

missing DLL library functions within library.py #740

Closed

		@@ -5,32 +5,36 @@ class ParameterUsageOptions(Enum):
		'''Different usage options for parameter lists.'''

		SESSION_METHOD_DECLARATION = 1

Change repeated capabilities #718

Change repeated capabilities #718

Conversation

texasaggie97-zz commented Jan 4, 2018

What does this Pull Request accomplish?

List issues fixed by this Pull Request below, if any.

What testing has been done?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thejcannon Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

texasaggie97-zz commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thejcannon Jan 16, 2018 •

edited

Loading