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

Implement the Correlator special suite #328

Draft
wants to merge 33 commits into
base: dev
Choose a base branch
from
Draft

Implement the Correlator special suite #328

wants to merge 33 commits into from

Conversation

JamesWrigley
Copy link
Member

@JamesWrigley JamesWrigley commented Dec 10, 2021

Finally got around to making a PR out of this 😅 I'm keeping it as a draft for now because there are a couple of (hopefully) minor issues still to fix:

  • f581c87 needs to be refactored, you can ignore this commit for now.
  • This uses the high_high_water_mark branch of metropc, which in turn is based on feat/bound_device_features. The high_high_water_mark branch just increases the HWM for the output socket of metropc, because I noticed that sometimes metropc was just dropping outputs and that fixed the issue. It could also be because the special suite is not reading fast enough, but that needs to be investigated. And perhaps we should wait to merge this until the feat/bound_device_features branch is merged too.

To reviewers, even though this is a draft I would suggest starting now because there's... a lot... of code. There's also some auxiliary commits that were needed for the Correlator, so I would suggest reviewing each commit individually because they're fairly atomic (it'll make more sense that way).

Note: this PR depends on #326, so you should ignore commits 1581c1b and 479e93a.

@lgtm-com
Copy link

lgtm-com bot commented Dec 10, 2021

This pull request introduces 1 alert when merging 5dbc0f1 into 2331f1d - view on LGTM.com

new alerts:

  • 1 for Inefficient regular expression

@lgtm-com
Copy link

lgtm-com bot commented Dec 20, 2021

This pull request introduces 1 alert when merging e5be471 into 2331f1d - view on LGTM.com

new alerts:

  • 1 for Inefficient regular expression

@JamesWrigley
Copy link
Member Author

Hmm, it's failing because the CI can't install metropc. Currently it's using an SSH URL but the repo is private anyway so using HTTPS won't help. @philsmt, is there a timeline to make metropc public?

@lgtm-com
Copy link

lgtm-com bot commented Dec 20, 2021

This pull request introduces 1 alert when merging 4288618 into f8d225d - view on LGTM.com

new alerts:

  • 1 for Inefficient regular expression

metadata for plotting.
"""
xr_attrs = {
"xlabel": xlabel,
Copy link

Choose a reason for hiding this comment

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

When receiving data as an xarray.DataArray (as you create here), there is already a documented protocol in metropc to use the coordinate label(s) as X axis label in 1D, whereas the y label is passed as an attribute (like here).

Would you mind using this instead of an xlabel attribute for compatibility?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah sure, that should be fine.

@lgtm-com
Copy link

lgtm-com bot commented Feb 3, 2022

This pull request introduces 1 alert when merging c27cd0c into 1489977 - view on LGTM.com

new alerts:

  • 1 for Inefficient regular expression

@lgtm-com
Copy link

lgtm-com bot commented Feb 16, 2022

This pull request introduces 1 alert when merging 46086ae into 86177ea - view on LGTM.com

new alerts:

  • 1 for Inefficient regular expression

@lgtm-com
Copy link

lgtm-com bot commented Feb 17, 2022

This pull request introduces 2 alerts when merging e685bbe into 621f1e5 - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Inefficient regular expression

@lgtm-com
Copy link

lgtm-com bot commented Mar 4, 2022

This pull request introduces 2 alerts when merging 2759386 into 7b704c3 - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Inefficient regular expression

@lgtm-com
Copy link

lgtm-com bot commented Mar 16, 2022

This pull request introduces 4 alerts when merging 4b8f0e0 into 56f56f5 - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for Unused local variable
  • 1 for Inefficient regular expression

This is to match what the DataTransformer does.
This makes it easier to change the settings from within special suites, without
having to dig into _SpecialAnalysisBase code.
@lgtm-com
Copy link

lgtm-com bot commented May 6, 2022

This pull request introduces 4 alerts when merging 3a135f8 into b9e30b5 - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for Unused local variable
  • 1 for Inefficient regular expression

This is a very simple implementation of something that can hopefully be improved
and cleaned up in the future: support for context-defined annotation widgets.

Each ROI is defined as a parameter, they are added to a View with an argument to
the Image_WithRoi type, which is read by the Correlator and thus creates an
ROI. When the ROI is changed, an updated parameter is sent to metropc and the
Correlator modifies the context file with the new ROI parameters (position,
height, etc).
Also added a warning dialog that appears when the user tries to move a ROI when
there's a syntax error in the code (which libcst will fail to parse).
Among other things, this makes it possible to downsample all series being
plotted.
This is to allow LinearROIs on images.
This is in preparation for using another library that's most up-to-date on
conda.
- Replace np.warnings with the warnings module
- Replace np.float with float
- Pass enum values instead of the enums themselves to redis functions
- Drop support for saving float arrays PNG images and expecting them to be
  restored with the same values. Seems that Pillow dropped support for it.
Lots of minor fixes to the tests, and fixed one bug in the correlator that
caused an extra nan value to be added to the error arrays for each series.
@philsmt
Copy link

philsmt commented Jul 28, 2023

We should probably aim to point this at metropc's main branch 🤔

@philsmt philsmt closed this Jul 28, 2023
@JamesWrigley
Copy link
Member Author

I think you pressed the wrong button there 😄

@JamesWrigley JamesWrigley reopened this Jul 28, 2023
@philsmt
Copy link

philsmt commented Jul 28, 2023

I did, apologies 😳

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.

3 participants