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

ES-223: Implement SubChannelVisibility feature #150

Merged
merged 5 commits into from
Apr 14, 2024

Conversation

StokesMIDE
Copy link
Member

This adds parsing of the <SubChannelVisibility> element, and the ability to filter the results of Dataset.getPlots() (used by enDAQ Lab to retrieve the subchannels to draw) by visibility level. By default, anything with a visibility level greater than zero is excluded.

Without additional changes to enDAQ Lab, this will hide all channels we mark as 'invisible,' but with no mechanism for displaying them. Future versions of enDAQ Lab will make a range of the invisible subchannels (e.g., GPS time and other data meaningful to a human user) available in the 'View->Display Channels' submenu. An 'advanced option' will make it possible to display any invisible channel (e.g., diagnostic data, the plot of which is largely meaningless).

@codecov-commenter
Copy link

codecov-commenter commented Apr 9, 2024

Codecov Report

Attention: Patch coverage is 94.11765% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 77.61%. Comparing base (6420d42) to head (13c6b69).
Report is 5 commits behind head on develop.

Files Patch % Lines
idelib/dataset.py 94.11% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #150      +/-   ##
===========================================
+ Coverage    74.28%   77.61%   +3.32%     
===========================================
  Files           10       11       +1     
  Lines         3691     4436     +745     
===========================================
+ Hits          2742     3443     +701     
- Misses         949      993      +44     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pscheidler
Copy link
Member

I plan to approve but 1 questions before this gets merged in and falls off my radar:

  • Will users be able to export the data, even though they can't see it in a plot?

@StokesMIDE
Copy link
Member Author

* Will users be able to export the data, even though they can't see it in a plot?

As currently implemented in enDAQ Lab, all channels should appear in the export dialog, since that list is built from a different source - directly from the channel list, not getPlots(). A future version might have some of the more technical/diagnostic channels hidden unless a 'show advanced' option is checked.

Copy link
Contributor

@tgipsonMIDE tgipsonMIDE left a comment

Choose a reason for hiding this comment

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

Looks good to me! Quick caveat though, all of the time correlation (etc.) channels written by the device currently have visibility +50. If we want them to be +1 going forward it's an easy change, we'll just have to update the FW and docs to reflect it.

@StokesMIDE
Copy link
Member Author

Looks good to me! Quick caveat though, all of the time correlation (etc.) channels written by the device currently have visibility +50. If we want them to be +1 going forward it's an easy change, we'll just have to update the FW and docs to reflect it.

@tgipsonMIDE I'm happy to do whatever scale/range, and it's trivial for me to change things on my end. I dug out a proposed range; does it look like what you'd planned?

0: Always visible (default)

10: Optionally visible, things probably of interest to the user in certain situations

20: Hide by default, but possibly of use to the user

30: Hide by default, channel used indirectly (but human readable)

40: Hide by default, channel used indirectly (not very human readable)

50: Diagnostic, always hide. Accessible via the idelib API.

@StokesMIDE StokesMIDE requested a review from tgipsonMIDE April 11, 2024 22:13
@tgipsonMIDE
Copy link
Contributor

@tgipsonMIDE I'm happy to do whatever scale/range, and it's trivial for me to change things on my end. I dug out a proposed range; does it look like what you'd planned?

It works for me, and is better than what's currently documented (that I know of), i.e. 50 = "hidden" and all other values are undefined. Would 30 then be a better fit for time-sync channels? If so, I can add a ticket to change it (keep 50 for NMEA data). I still like the idea of differentiating why a channel is hidden, but if this value deals strictly with plot visibility, other concerns (export visibility, machine-read output) could be handled by other mechanisms.

@StokesMIDE StokesMIDE merged commit afcc8b0 into develop Apr 14, 2024
35 checks passed
@StokesMIDE StokesMIDE deleted the feature/ES-223_channel_visibility branch April 14, 2024 04:32
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.

4 participants