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

Enable Single Regression in RSNRegression #170

Merged
merged 2 commits into from
Apr 2, 2020
Merged

Conversation

glasserm
Copy link
Contributor

@glasserm glasserm commented Apr 2, 2020

Enable a new 'single' mode of temporal regression for RSNRegression to enable applying existing results to a new CIFTI space or single regression in other contexts

Enable a new 'single' mode of temporal regression for RSNRegression to enable applying existing results to a new CIFTI space or single regression in other contexts
otherwise
error(['unrecognized method: "' Method '", use "weighted" or "dual"']);
error(['unrecognized method: "' Method '", use "weighted," "dual," or "single"']);
Copy link
Member

Choose a reason for hiding this comment

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

I would put the commas outside the quotes to reduce confusion, this isn't a manuscript.

Comment on lines 231 to 232
matlab_args+=", 'GroupMaps', '$GroupMaps'"
matlab_args+=", 'VAWeightsName', '$tempfile.91k.dscalar.nii'"
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: indentation

@glasserm
Copy link
Contributor Author

glasserm commented Apr 2, 2020

Any other comments?

@coalsont
Copy link
Member

coalsont commented Apr 2, 2020

Personally, I wouldn't have reused SpectraParams to specify an input file in this mode.

@glasserm
Copy link
Contributor Author

glasserm commented Apr 2, 2020

Since it is exclusive, already there, and germane it seemed faster.

@glasserm
Copy link
Contributor Author

glasserm commented Apr 2, 2020

Since this is testing and working and I want to run HCP Processing with it now, I will go ahead and commit it.

@glasserm glasserm merged commit a4728cc into master Apr 2, 2020
@glasserm glasserm deleted the Single-Regression branch April 2, 2020 01:18
@mharms
Copy link
Contributor

mharms commented Apr 2, 2020

We need to be careful with the versioning. version.txt needs to be updated to Post-v4.2.0 as part of this. Relatedly, the "rc.1" tag from v4.2.0 can be dropped, and v4.2.0 released.

@glasserm
Copy link
Contributor Author

glasserm commented Apr 2, 2020

That will be annoying to change the version with every commit. Can we do something more automated?

@coalsont
Copy link
Member

coalsont commented Apr 2, 2020

If it needs to work in the zip files offered on the releases page, then not really.

@glasserm
Copy link
Contributor Author

glasserm commented Apr 2, 2020

How do we handle this in Connectome Workbench?

@coalsont
Copy link
Member

coalsont commented Apr 2, 2020

Manually there also, since debian doesn't keep the git stuff in the source tree.

@mharms
Copy link
Contributor

mharms commented Apr 2, 2020

Ok, I just noticed that this was a change to a .m file. @glasserm : I've made this point before, but we can't be making changes to matlab code and merging it into the master without simultaneously recompiling the matlab. We now have a situation in which the compiled matlab is not in sync with the interpreted matlab. I'll leave it to you to work with Mike Hodge to resolve this. IMO, it would have been better to leave this change in a branch and use that for your immediate needs. Then we could deal with the compiled matlab issue on a more leisurely time frame.

@glasserm
Copy link
Contributor Author

glasserm commented Apr 2, 2020

I don't think we should support compiled matlab then.

@coalsont
Copy link
Member

coalsont commented Apr 2, 2020

That script doesn't currently support compiled matlab, as it happens, so it technically isn't out of sync.

@mharms
Copy link
Contributor

mharms commented Apr 2, 2020

That's not a solution. The solution is simply to make use of a branch in your processing.

@glasserm
Copy link
Contributor Author

glasserm commented Apr 2, 2020

The feature is ready for routine use by everyone. Unless compiled matlab can be automatically built upon commit (like Connectome Workbench is), it isn't feasible to support. We have already seen that compiled matlab has major problems with memory usage. We should be supporting interpreted matlab and a recipe for building Octave without problems + the container.

@mharms
Copy link
Contributor

mharms commented Apr 2, 2020

I have no doubt that Octave is going to have a host of problems that we are completely clueless about currently. Dropping support for compiled matlab at this point in time would likely be a disaster. Fortunately, as @coalsont pointed out, RSNRegression doesn't even have a compiled counterpart at this point in time. If dropping support for compiled matlab is a goal, then someone is going to have to put in a bunch of time validating all the code in Octave. Until that is done, changes to matlab scripts that have a compiled version need to be accompanied by an updated compiled binary as well.

@glasserm
Copy link
Contributor Author

glasserm commented Apr 2, 2020

That is not feasible without an automated build system. It is reasonable for releases but not for every commit to master.

@mharms
Copy link
Contributor

mharms commented Apr 2, 2020

The feature is ready for routine use by everyone.

BTW: I think you considerably overestimate the number of people that are running the matlab stuff in interpreted mode. Most people using a cluster are probably going to need to be using either compiled matlab or Octave.

@mharms
Copy link
Contributor

mharms commented Apr 2, 2020

That is not feasible without an automated build system. It is reasonable for releases but not for every commit to master.

Yes, it isn't ideal to have to worry about keeping things in sync, but we can't drop compiled matlab at this point in time; not if you want others to be able to use those scripts on their cluster computing systems (or even just without needing a potentially expensive matlab license on their local computer).

@glasserm
Copy link
Contributor Author

glasserm commented Apr 2, 2020

I don't really have anything else to say about this. Things will not be in sync in master except before releases or we need an automated build system. No one manually compiles software and uploads it to a repo with every commit.

@coalsont
Copy link
Member

coalsont commented Apr 2, 2020

I think the point of the master matlab compile script was so that when we are about ready to release, we can get them all synced then. Does our documentation give the impression that the master branch is the first version for users to try?

@mharms
Copy link
Contributor

mharms commented Apr 2, 2020

No one manually compiles software and uploads it to a repo with every commit.

FWIW, the binaries really shouldn't be part of the repo anyway, but that's an organizational discussion for another time. Storing a history of all the binary changes as part of the repo bloats its size.

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