Conversation
|
I could have sworn that this MR is open for a longer time, at least I'm sure I've looked at the code soon after you committed in July. LGTM 🙂 |
| self._abs_difference_cb.setChecked(True) | ||
|
|
||
| self._reset_btn = QPushButton("Reset") | ||
| self.save_btn = QPushButton("Save to file") |
There was a problem hiding this comment.
would rather prefer a QAction in a toolbar on this window instead of a button to save to files.
There was a problem hiding this comment.
Hmm, @CammilleCC I leave this up to you. I think either UI is acceptable.
| if not len(self._queue): | ||
| return |
There was a problem hiding this comment.
It would be better if the button itself was disabled when there is no data to save, rather than silently exiting.
| # Get the current data | ||
| if not len(self._queue): | ||
| return | ||
| data = self._queue[0] |
There was a problem hiding this comment.
This changed in #318, the GUI now receives a dict of all the data from the workers. So here we'd now need self._queue[0]["processed"].
| # Copy reference file from tmp folder to desired destination | ||
| os.makedirs(osp.dirname(filepath), exist_ok=True) |
There was a problem hiding this comment.
This comment doesn't seem to match the code? And why is it necessary to use os.makedirs() here? I thought we could trust that any directory returned by QFileDialog.getSaveFileName() already exists since the user would need to create it first in the dialog.
| intensity_sub = pp.y | ||
|
|
||
| if position is None: | ||
| return |
There was a problem hiding this comment.
I think this should show an error dialog to make it clear to the user that nothing happened.
| intensity_subtracted=intensity_sub) | ||
| elif filter is SaveFile.TXT: | ||
| # write out conversion to tabular ASCII | ||
| with open(filepath, 'w') as f: |
There was a problem hiding this comment.
Could we use Python's csv library here?
|
Apologies for my shockingly slow review 🙈 |
4 participants
SPB requested to have a saved file for the pump-probe values. We now offer an ASCII and an NPZ output.