-
Notifications
You must be signed in to change notification settings - Fork 1
Arpes gui #35
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
base: main
Are you sure you want to change the base?
Arpes gui #35
Conversation
…ant results for post processing in Jupyter Notebook
…e fit panel, add more comments to the code
…mall bug the fit panel
…function (to extract the data of the graph)
…a cursors for the integration range
rettigl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address the few points that I have noted here, then we can finish this finally for now.
| if self.vmax<10: | ||
| self.cmin, self.cmax = 10, 1e9 | ||
| self.case=True | ||
| else: | ||
| self.cmin, self.cmax=self.vmin,self.vmax |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this here? I don't understand this.
| if self.case : | ||
| self.slider.setValue([self.new_values(self.vmin), self.new_values(self.vmax)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this. Please explain, and give meaningful variable names
| self.slider.setValue([float(self.vmin),float(self.vmax)]) | ||
| if self.case : | ||
| self.slider.setValue([self.new_values(self.vmin), self.new_values(self.vmax)]) | ||
| self.slider.valueChanged.connect(lambda value: self.update_clim(value)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this lambda function, can't you use the function directly?
| if self.active_cursor == self.artist: | ||
| if self.artist.get_xdata()[0]==self.artist.get_xdata()[1]: | ||
| self.artist.set_xdata([event.xdata, event.xdata]) | ||
| self.changes() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this work, if changes=None?
Also, please give meaningful names
| self.fig.canvas.mpl_connect('pick_event', self.on_pick) | ||
| self.fig.canvas.mpl_connect('motion_notify_event', self.on_motion) | ||
| self.fig.canvas.mpl_connect('button_release_event', self.on_release) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you connect these handlers in each instance, this means you will have a lot of handlers connected to each event happening on the canvas. I am wondering if this might slow things down, compared to only one global handler per event type.
| npts = min(arr.size, kernel.size) | ||
| pad = np.ones(npts) | ||
| tmp = np.concatenate((pad*arr[0], arr, pad*arr[-1])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this only works if kernel size < arr size
| return out[noff:noff+npts] | ||
|
|
||
|
|
||
| def convolution(x, func, *args, sigma=1.0): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this in addition? Which one do you use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to remove this also for now. It does not make sense to add 1200 lines twice. This cannot in any way be maintained.
| def __init__(self, canvas, ax, show_popup=None): | ||
| self.canvas = canvas | ||
| self.ax = ax | ||
| self.show_popup=show_popup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you connect the triggers here, as for the other handlers?
| # Create a figure and canvas for the graph | ||
| figure, axis = plt.subplots(figsize=(20, 20)) | ||
| figure, axis = plt.subplots(figsize=(10, 10)) | ||
| plt.close(figure) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have to do this with plt.ioff()?
No description provided.