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

Added 20*log10(mag) for Pyqtgraph #395

Merged
merged 20 commits into from
Jun 14, 2023

Conversation

jeremiahnlin
Copy link
Contributor

@jeremiahnlin jeremiahnlin commented May 26, 2023

Added 20*log10(mag)/phase graphs for Pyqtgraph backend for plottr-monitr

closes #224

Copy link
Collaborator

@marcosfrenkel marcosfrenkel left a comment

Choose a reason for hiding this comment

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

The logmag view works great when the data is being plotted in a 2 linear plot. However when you have a 2D plot, the view does not seem to be doing anything. I don't think that needs to work for a 2D plot, but if possible, the option to plot it in that way in a 2D plot should be disabled if it doesn't get fixed

@@ -264,6 +270,11 @@ class ComplexRepresentation(LabeledOptions):
#: magnitude and phase
magAndPhase = "Mag/Phase"

#: Natural Logarithmic magnitude and phase
log10_MagAndPhase = "20*log10(Mag)/Phase"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would just rename it to logMag/Phase or something like that. Like this seems to long for me

@marcosfrenkel
Copy link
Collaborator

marcosfrenkel commented May 31, 2023

@wpfff this would close issue #224 right?

Copy link
Collaborator

@marcosfrenkel marcosfrenkel left a comment

Choose a reason for hiding this comment

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

The code is behaving exactly how I thought it would! For some reason something is crashing in the background. It is not stopping it from working, but we should not merge something that is crashing


#Switch the 1d plots to the logarithmic variation
if plotItem.plotDataType == PlotDataType.scatter1d and plotItem.subPlot == 0: plotItem.plotDataType = PlotDataType.log10_scatter1d
if plotItem.plotDataType == PlotDataType.line1d and plotItem.subPlot == 0: plotItem.plotDataType = PlotDataType.log10_line1d
Copy link
Collaborator

Choose a reason for hiding this comment

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

convert these 2 lines into 4. There is no need to save on vertical space

@@ -212,11 +219,22 @@ def _1dPlot(self, plotItem: PlotItem) -> None:
return subPlot.plot.plot(x.flatten(), y.flatten(), name=name,
pen=mkPen(color, width=1), symbol=symbol, symbolBrush=color,
symbolPen=None, symbolSize=symbolSize)
else:
elif plotItem.plotDataType == PlotDataType.log10_line1d:
name = plotItem.labels[-1] if isinstance(plotItem.labels, list) else ''
Copy link
Collaborator

Choose a reason for hiding this comment

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

This syntax is cool, but it makes it harder to read compared to a normal if else block

@@ -361,6 +396,10 @@ class FigureOptions:
#: how to represent complex data
complexRepresentation: ComplexRepresentation = ComplexRepresentation.realAndImag

numAxes: int = 0

imagData: bool = False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should there be comments explaining what they are? they look pretty self explanatory but maybe?

if not val.imag == 0:
self.figOptions.imagData = True
break
if not all(val.imag == 0):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line is currently crashing on my machine

Copy link
Collaborator

@marcosfrenkel marcosfrenkel 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!

@marcosfrenkel marcosfrenkel merged commit ed6c541 into toolsforexperiments:master Jun 14, 2023
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.

add more options for complex data
2 participants