Skip to content

Conversation

@texasaggie97-zz
Copy link
Contributor

  • 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?

  • Add fancy wrapper around get_pin_results_pin_information()
    • Returns named tuple PinInfo(pin_indexes, site_numbers, channel_indexes)

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

What testing has been done?

  • Visual inspection of generated code

@codecov
Copy link

codecov bot commented Nov 6, 2019

Codecov Report

Merging #1098 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1098   +/-   ##
=======================================
  Coverage   89.99%   89.99%           
=======================================
  Files          20       20           
  Lines        3679     3679           
=======================================
  Hits         3311     3311           
  Misses        368      368

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 004c7ae...a7ee3ea. Read the comment docs.

sbethur
sbethur previously requested changes Nov 6, 2019
@texasaggie97-zz texasaggie97-zz dismissed sbethur’s stale review November 7, 2019 16:22

Comments addressed

Fields in Waveform:
- **site** (int)
- **data** (array.array of int)
Copy link
Member

Choose a reason for hiding this comment

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

  • Is array.array of int the right type for the data of digital waveforms?
  • When I think of Waveform I think of an analog signal by default. Should we call this DigitalWaveform or is it enough that it is an nidigital.Waveform?

'''fetch_capture_waveform
TBD
Returns a list of named tuples (Waveform) that <FILL IN THE BLANK HERE>
Copy link
Member

Choose a reason for hiding this comment

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

We should add an issue about <FILL IN THE BLANK HERE> unless we have another way to track it in place.

Returns:
data (list of int):
waveform (list of Waveform): List of named tuples with fields:
Copy link
Member

Choose a reason for hiding this comment

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

Is the order important?
If not, it might make sense for this to be a dictionary of site->waveform data
And perhaps we should consider replacing the name tuple with the same... one less type.


data, actual_num_waveforms, actual_samples_per_waveform = self._fetch_capture_waveform(site_list, waveform_name, samples_to_read, timeout)
timeout_secs = _converters.convert_timedelta_to_seconds(timeout, _visatype.ViReal64)
data, actual_num_waveforms, actual_samples_per_waveform = self._fetch_capture_waveform(site_list, waveform_name, samples_to_read, timeout_secs)
Copy link
Member

Choose a reason for hiding this comment

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

Why is _fetch_capture_waveform not code-generated?
The comment doesn't say so.

${helper.get_function_docstring(f, False, config, indent=8)}
'''
import collections
import sys
Copy link
Member

Choose a reason for hiding this comment

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

Can't leave a comment on line 3
'''Dispatches to the appropriate "fetch waveform into" method based on the waveform type.'''

That seems wrong.

@texasaggie97-zz texasaggie97-zz changed the title [#1096] Make get_pin_results_pin_information() fancy [#1112] Make get_pin_results_pin_information() fancy Nov 12, 2019
@texasaggie97-zz
Copy link
Contributor Author

Replaced by #1120 to get rid of other files

@texasaggie97-zz texasaggie97-zz deleted the bug1089/fancy_get_pin_results branch November 13, 2019 21:48
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.

Make get_pin_results_pin_information in nidigital API fancy

4 participants