-
Notifications
You must be signed in to change notification settings - Fork 13
Fixes #799 Make import a full module #872
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
Conversation
|
@MichaelStritt @jan-petr Why do we want the |
HenkMutsaerts
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.
Let's wait with this and discuss properly, there are easier/less custom ways to do this, it seems.
We've actually discussed this already on Monday. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@jan-petr: The funny thing right now is that it crashes a few times and you get a nice amount of warnings, but the results are still fine. Let me explain. Right now |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
jan-petr
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.
Minor changes only. Additionally - I don't want to add the lock files and log files to derivativesReference (because these can potentially change and we don't really need a reference for these). With new derivativesReferences, I will not add it to Flavors, but we need to make sure that the compare-differences ignores them.
I am now running the full-flavor test to see if all works.
This comment has been minimized.
This comment has been minimized.
Don't use filesep, but fullfile - that's OS non-specific... also - Philips_PCASL_2DEPI_PARREC_1 did create temp-files, but never rawdata. |
jan-petr
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.
Philips_PCASL_2DEPI_PARREC_1 doesn't work - rawdata not created and subsequently, the comparison script crashes.
@jan-petr: the problem was that I seemingly added a superfluous underscore because of the _1 in the tokens. It should work now. I also fixed the escaping, which should basically only be necessary for windows anyway. af1e66b 👍 |
This comment has been minimized.
This comment has been minimized.
Yeah, wasn't my intention to leave that in there. There's probably just a small mistake somewhere where I do the regular expressions. I'll give it a quick try on the AMC server. Edit: Oh, okay, so this goes wrong for Koens Hadamard dataset? Is the import already working for that one? Edit 2: @jan-petr better after this d37c17c & e118d58 ? I just did a stupid mistake with the escaping it seems... |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Known issues:
|
55943e2 to
35745b9
Compare
Linked issue
Check out #799
How to test
Required: if not defined in the linked issue, add a simple test description here
Comments
Optional: add helpful comments for the reviewers here