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

5853 plot annotations prototype #6000

Merged
merged 87 commits into from Jan 20, 2023
Merged

Conversation

scottbell
Copy link
Contributor

@scottbell scottbell commented Nov 21, 2022

Closes #5853

Describe your changes:

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

  • Changes address original issue?
  • Tests included and/or updated with changes?
  • Command line build passes?
  • Has this been smoke tested?
  • Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

  • Changes appear to address issue?
  • Reviewer has tested changes by following the provided instructions?
  • Changes appear not to be breaking changes?
  • Appropriate automated tests included?
  • Code style and in-line documentation are appropriate?
  • Has associated issue been labelled unverified? (only applicable if this PR closes the issue)
  • Has associated issue been labelled bug? (only applicable if this PR is for a bug fix)

scottbell and others added 30 commits May 12, 2022 14:11
@scottbell
Copy link
Contributor Author

scottbell commented Jan 20, 2023

@akhenry @shefalijoshi Another round of changes. Annotations should now be only be editable/viewable in fixed time mode. Also, I've generally improved plot performance in fixed time mode. Please double check that plot performance is similar to master with no annotations added. Thank you for the review!

@akhenry
Copy link
Contributor

akhenry commented Jan 20, 2023

This is looking really great, thank you!

In testing this I think we maybe want to allow annotations in real-time mode if the plot is paused. Fine to do that in a followup though if it's a lot of work.

Copy link
Contributor

@akhenry akhenry left a comment

Choose a reason for hiding this comment

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

I have done some testing of this against real telemetry points in the viper test environment and discovered that Yamcs parameters cannot be annotated because the domain objects are immutable. Our implementation appears to be trying to mutate the telemetry points themselves?

I am unable to add a tag to an overlay plot containing a Yamcs telemetry point and I get the following error:

Screen Shot 2023-01-20 at 7 37 42 AM

@scottbell
Copy link
Contributor Author

scottbell commented Jan 20, 2023

I have done some testing of this against real telemetry points in the viper test environment and discovered that Yamcs parameters cannot be annotated because the domain objects are immutable. Our implementation appears to be trying to mutate the telemetry points themselves?

I am unable to add a tag to an overlay plot containing a Yamcs telemetry point and I get the following error:

Screen Shot 2023-01-20 at 7 37 42 AM

Yes, we have a item called annotationLastCreated on the domain objects. Views (like the annotation inspector and the notebook) can watch this domain object and be notified when new annotations are created (and usually call getAnnotations to get the new annotations).

The mutation is happening here. Note we had this with the Notebook tags too, but the notebook domain objects weren't immutable.

@akhenry
Copy link
Contributor

akhenry commented Jan 20, 2023

For now, let's defer the problem of observing for annotation changes on immutable objects. We'll need to come up with a clever solution though...

Copy link
Contributor

@akhenry akhenry left a comment

Choose a reason for hiding this comment

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

Found a few minor issues in testing, will file followups. Thanks!

@akhenry akhenry enabled auto-merge (squash) January 20, 2023 20:50
Copy link
Contributor

@shefalijoshi shefalijoshi left a comment

Choose a reason for hiding this comment

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

LGTM

@akhenry akhenry merged commit d1c7d13 into master Jan 20, 2023
@shefalijoshi shefalijoshi deleted the 5853-plot-annotations-prototype branch January 20, 2023 22:34
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.

Plot Annotations Prototype
4 participants