nidcpower advanced sequencing #957

texasaggie97-zz · 2018-11-08T17:20:53Z

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

What does this Pull Request accomplish?

Implements more Pythonic API for nidcpower advanced sequences
- Option 1 from Expose NI-DCPower Advanced Sequences in Python API #504
Documentation and tests still remaining but looking for feedback before doing that work

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

Fix Expose NI-DCPower Advanced Sequences in Python API #504

What testing has been done?

codecov · 2018-11-08T17:33:06Z

Codecov Report

Merging #957 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #957   +/-   ##
=======================================
  Coverage   83.82%   83.82%           
=======================================
  Files          26       26           
  Lines        5742     5742           
=======================================
  Hits         4813     4813           
  Misses        929      929

Flag	Coverage Δ
#codegenunittests	`87.76% <0.00%> (ø)`	⬆️
#nidcpowersystemtests	`48.93% <0.00%> (ø)`	⬆️
#nidigitalsystemtests	`51.72% <0.00%> (ø)`	⬆️
#nifakeunittests	`96.06% <0.00%> (ø)`	⬆️
#nifgensystemtests	`96.91% <0.00%> (ø)`	⬆️
#nimodinstsystemtests	`100.00% <0.00%> (ø)`	⬆️
#nimodinstunittests	`95.98% <0.00%> (ø)`	⬆️
#nisesystemtests	`100.00% <0.00%> (ø)`	⬆️
#nitclksystemtests	`100.00% <0.00%> (ø)`	⬆️
#nitclkunittests	`100.00% <0.00%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3102037...3102037. Read the comment docs.

generated/nidcpower/session.py

epage · 2018-11-08T17:32:43Z

generated/nidcpower/session.py

+
+        Test.
+
+        Note: Test.


As you mention, documentation is still outstanding

I don't want to put in the effort to fix it in case we end up doing something different.

Sounds like documentation isn't fixed. I'm unresolving this to help us, the reviewers, track this being resolved.

generated/nidcpower/session.py

src/nidcpower/metadata/functions_addon.py

texasaggie97-zz · 2018-11-09T14:53:11Z

How do we handle channels?

Do we require the call happen on the channels rep_cap container, even though create_advanced_sequence and create_advanced_sequence_step themselves don't take a channel string?

Can the channel change per step? Or in a single step, can there be different configurations per channel? If so, option 1 doesn't work as is.

marcoskirsch · 2018-11-09T20:14:28Z

After some experimentation.

How to get the attribute ID given the Python name of the attribute, DRY version:

>>> nidmm.Session.__bases__[0].__dict__['range']._attribute_id
1250002

How to set an attribute given the Python name of the attribute, DRY version:

>>> setattr(s, 'range', 100)
>>> s.range
100.0

I like this better than duplicating known information in the code, and adding complexity to the mako template.

The drawback is that this requires knowledge of the "private" aspects of Attribute. I think this is ok, it's all within the same module and test suite.

marcoskirsch · 2018-11-09T20:24:05Z

How do we handle channels?

Do we require the call happen on the channels rep_cap container, even though create_advanced_sequence and create_advanced_sequence_step themselves don't take a channel string?

Can the channel change per step? Or in a single step, can there be different configurations per channel? If so, option 1 doesn't work as is.

NI-DCPower only allows sequences in single-channel sessions, so we're ok not passing a channel.

generated/nidcpower/session.py

src/nidcpower/metadata/functions_addon.py

marcoskirsch · 2019-08-30T18:33:19Z

I think we should close this PR until it's ready to be looked at.

Comments addressed

marcoskirsch

This doesn't support "commit step" functionality that was added to NI-DCPower recently. I think that's ok, it can be added in a separate PR but we should make sure it's tracked with an issue.

This feature may warrant an example.

I'm very concerned that I don't see how this API can accomodate multi-channel sequences should support be added. Do you have any ideas?

build/templates/tox-system_tests.ini.mako

marcoskirsch · 2020-02-21T20:09:28Z

generated/nidcpower/nidcpower/session.py

+        for step in sequence:
+            for key in step:
+                if key not in Session.__base__.__dict__:
+                    raise TypeError('{0} is not an property on the nidcpower Session class'.format(key))


Why TypeError and not KeyError?
the nidcpower Session class -> nidcpower.Session

KeyError is an implementation detail, and I thought TypeError more appropriate. And to be consistent with the other error condition. Since I don't have a strong opinion though, do you want me to change it?

Changed to nidcpower.Session

I think it's more correct to return KeyError when the name is bogus and TypeError when the name is for something that exists but isn't a property.

generated/nidcpower/nidcpower/session.py

generated/nitclk/tox-system_tests.ini

src/nidcpower/metadata/functions.py

marcoskirsch · 2020-02-24T22:44:04Z

Closing this based on #504 (comment)
We're implementing #1289 instead. :(

texasaggie97-zz added 4 commits November 8, 2018 11:16

Add fancy wrapper for DCPower advanced sequencing

63705bc

Make delete_advanced_sequence public

4ca131c

Fix comments

2d0e31c

Update generated files

093a01b

texasaggie97-zz added this to the nimi-python 1.2 release milestone Nov 8, 2018

texasaggie97-zz self-assigned this Nov 8, 2018

texasaggie97-zz requested review from epage and marcoskirsch November 8, 2018 17:20

epage suggested changes Nov 8, 2018

View reviewed changes

marcoskirsch reviewed Nov 8, 2018

View reviewed changes

texasaggie97-zz added 2 commits November 8, 2018 15:28

Store the set function to use instead of trying to store the property

0d7bbd9

Update generated files

be4c89f

Fix test

0ea477d

texasaggie97-zz added 3 commits November 9, 2018 14:50

Look into Session to get the attribute id.

7f00575

Update the test to use the new API

36425f4

Update generated files

251c1d9

marcoskirsch reviewed Nov 12, 2018

View reviewed changes

generated/nidcpower/session.py Outdated Show resolved Hide resolved

marcoskirsch previously requested changes Nov 12, 2018

View reviewed changes

generated/nidcpower/session.py Outdated Show resolved Hide resolved

generated/nidcpower/session.py Outdated Show resolved Hide resolved

generated/nidcpower/session.py Outdated Show resolved Hide resolved

src/nidcpower/metadata/functions_addon.py Outdated Show resolved Hide resolved

texasaggie97-zz added 5 commits July 24, 2019 14:00

Merge branch 'master' into bug504/advanced_sequencing

8c1cb1a

Add 'FanceCreateAdvancedSequenceStep' to funcitons since not in addon

bbde49c

delete_advances_sequence is now public

1043b21

Updates based on comments

d651183

Update generated files

4daa7ee

MrDuff8 modified the milestones: nitclk early access, nidcpower support for Advanced Sequences Aug 21, 2019

texasaggie97-zz added 7 commits January 7, 2020 15:11

Merge branch 'master' into bug504/advanced_sequencing

135ea6c

Update generated files

2407384

Merge branch 'master' into bug504/advanced_sequencing

350d007

Merge branch 'master' into bug504/advanced_sequencing

9e953ee

DeleteAdvancedSequence is now public

0edfc86

Add error checking on attribute name.

06bce2f

Update generated files

dd2a98d

texasaggie97-zz added 8 commits February 18, 2020 14:53

Check name and type separately

c61abc0

update generated files

52a7d3e

Fix logic error

47dc358

Fix if condition

37859c4

Update generated files

3985bea

Merge branch 'fix-merge-error' into bug504/advanced_sequencing

d572d03

Merge branch 'master' into bug504/advanced_sequencing

49a24ee

Start of advanced suqeunce example

69039b9

marcoskirsch requested changes Feb 21, 2020

View reviewed changes

texasaggie97-zz added 7 commits February 21, 2020 14:25

Fix description per review

d6d38ed

Update generated files

bf4b8bf

Merge branch 'master' into bug504/advanced_sequencing

1469fed

Update nidcpower_advanced_sequence.py

ab73c43

Change exception type

d4a5b62

Fix help text and function call

88781d4

Update generated files

3102037

marcoskirsch closed this Feb 24, 2020

marcoskirsch mentioned this pull request Feb 28, 2020

Advanced sequence multi channel #1289

Merged

3 tasks

sbethur deleted the bug504/advanced_sequencing branch March 22, 2020 05:06

marcoskirsch mentioned this pull request May 15, 2020

Replace usage of datetime library types with hightime types #1449

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nidcpower advanced sequencing #957

nidcpower advanced sequencing #957

texasaggie97-zz commented Nov 8, 2018 •

edited

codecov bot commented Nov 8, 2018 •

edited

epage Nov 8, 2018

epage Nov 8, 2018

texasaggie97-zz commented Nov 9, 2018 •

edited

marcoskirsch commented Nov 9, 2018 •

edited

marcoskirsch commented Nov 9, 2018

marcoskirsch commented Aug 30, 2019

marcoskirsch left a comment

marcoskirsch Feb 21, 2020

texasaggie97-zz Feb 21, 2020

marcoskirsch Feb 21, 2020

marcoskirsch commented Feb 24, 2020

nidcpower advanced sequencing #957

nidcpower advanced sequencing #957

Conversation

texasaggie97-zz commented Nov 8, 2018 • edited

What does this Pull Request accomplish?

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

What testing has been done?

codecov bot commented Nov 8, 2018 • edited

Codecov Report

epage Nov 8, 2018

Choose a reason for hiding this comment

epage Nov 8, 2018

Choose a reason for hiding this comment

texasaggie97-zz commented Nov 9, 2018 • edited

marcoskirsch commented Nov 9, 2018 • edited

marcoskirsch commented Nov 9, 2018

marcoskirsch commented Aug 30, 2019

marcoskirsch left a comment

Choose a reason for hiding this comment

marcoskirsch Feb 21, 2020

Choose a reason for hiding this comment

texasaggie97-zz Feb 21, 2020

Choose a reason for hiding this comment

marcoskirsch Feb 21, 2020

Choose a reason for hiding this comment

marcoskirsch commented Feb 24, 2020

texasaggie97-zz commented Nov 8, 2018 •

edited

codecov bot commented Nov 8, 2018 •

edited

texasaggie97-zz commented Nov 9, 2018 •

edited

marcoskirsch commented Nov 9, 2018 •

edited