Skip to content

Unifying GUI#728

Merged
dominikbach merged 31 commits intodevelopfrom
unifying-GUI
Jul 15, 2024
Merged

Unifying GUI#728
dominikbach merged 31 commits intodevelopfrom
unifying-GUI

Conversation

@dominikbach
Copy link
Copy Markdown
Contributor

@dominikbach dominikbach commented Jun 18, 2024

Stylises the Matlabbatch more uniformly.

Changes proposed in this pull request:

  • common selector functions for items that repeat across modules:
pspm_cfg_selector_channel
pspm_cfg_selector_channel_action
pspm_cfg_selector_data_design
pspm_cfg_selector_datafile
pspm_cfg_selector_filter
pspm_cfg_selector_norm
pspm_cfg_selector_outputfile
pspm_cfg_selector_overwrite
pspm_cfg_selector_python
pspm_cfg_selector_timeunits
  • common dependency creators for 3 types of dependencies: data files, model files, channel index
pspm_cfg_vout_modelfile
pspm_cfg_vout_outchannel
pspm_cfg_vout_outfile
  • separate run functions for all cfg functions
  • stylistic changes
  • removed some deprecated items
  • some bugfixes and improvements in the main PsPM functions

@dominikbach dominikbach requested a review from teddphil June 18, 2024 16:11
@dominikbach dominikbach self-assigned this Jun 18, 2024
@dominikbach dominikbach added the Completed & Waiting for Review Completed and waiting for review label Jun 18, 2024
@dominikbach dominikbach added this to the v7.0 milestone Jun 18, 2024
dadi.zhao added 2 commits June 24, 2024 12:49
I add a space to reveal this function for commenting, otherwise i cannot add comments for this function
This is to fix a conflict that is caused by my modification in a previous PR
@teddphil
Copy link
Copy Markdown

Hi @dominikbach apologies for uploading two commits to this PR.

  1. I need to add a comment to src/pspm_cfg/pspm_cfg_data_design_selector.m. This file was not changed in this PR so I have to add something to reveal it in change list for commenting. I only added a space that can be deleted later if you wish.
  2. I fixed the conflicts that are caused by PR 659. These changes only attempted to fix the conflicts and have no modifications to any change that is introduced in this PR.

@teddphil teddphil added Under Review and removed Completed & Waiting for Review Completed and waiting for review labels Jun 24, 2024
% missing epochs
if isfield(job.session(iSession).missing,'epochfile')
if isfield(job.session(iSession).missing,'epochfile')
model.missing{1,iSession} = job.session(iSession).missing.epochs.epochfile{1};
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this two lines look weird. If the intention is to check if there is a field epochfile under job.session(1).missing, the next line of code may want to say ... = job.session(1).missing.epochfile, yet it used job.session(1).missing.epochs.epochfile. This function reported an error in PR 729, because the detected epochfile was assigned to .missing instead of .missing.epochs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

At the line 331, the function uses pspm_cfg_python. This function has been renamed to pspm_cfg_selector_python, so the name of this function needs to be changed here too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed.

function out = pspm_cfg_python(varargin)
function out = pspm_cfg_selector_python(varargin)
% ● Description
% pspm_cfg_python is a function that provides UI controls for
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The help text needs to be updated too, maybe add some examples

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

image I think it is better to change the name of this item from "Path" to "Python Path"? "Path" indicates the path of "Bioread"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@teddphil
Copy link
Copy Markdown

teddphil commented Jul 15, 2024

I approved this PR as the test in GUI at my side seems to operate well. I will continue testing in the PR 721 later.
There are minor text adjustments, just please only modify if they are necessary. Thanks.

@teddphil teddphil added Approved The pull request has been approved and can be checked and then merged. and removed Under Review labels Jul 15, 2024
@dominikbach dominikbach merged commit 4e55a2c into develop Jul 15, 2024
@dominikbach dominikbach deleted the unifying-GUI branch July 15, 2024 11:23
@teddphil teddphil removed the Approved The pull request has been approved and can be checked and then merged. label Aug 5, 2024
@teddphil teddphil mentioned this pull request Oct 12, 2024
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.

2 participants