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

ENH: remove all prior existing citation files first #1567

Merged
merged 3 commits into from
Apr 5, 2019

Conversation

yarikoptic
Copy link
Contributor

  • makes it friendly to the cases where all output is under git-annex.
    .write_text() just opens a file in "w" mode so it would fail on
    annex protected file.

  • if previous run did succeed generating e.g. .html file, if subsequent run
    fails, updated version of .html would not be generated and provided
    CITATION files could get out of sync.

Removing all relevant CITATION* files prior generation resolves both of the issues.

No pressure -- if you don't like it just close it.

Comes not tested whatsoever ;-) (are there unittests to run for it?)

- makes it friendly to the cases where all output is under git-annex.
  .write_text() just opens a file in "w" mode so it would fail on
  annex protected file.

- if previous run did succeed generating e.g. .html file, if subsequent run
  fails, updated version of .html would not be generated and provided
  CITATION files could get out of sync.

Removing all CITATION* files prior generation resolves both of the issues.
They are suggestsions from code review by Oscar

Co-Authored-By: yarikoptic <debian@onerussian.com>
@yarikoptic yarikoptic force-pushed the enh-consistent-citation branch from da3cf26 to 19c7e71 Compare April 4, 2019 15:28
@oesteban
Copy link
Member

oesteban commented Apr 4, 2019

@yarikoptic, do you want to add yourself to the .zenodo.json file?

The order is not really clear, so added myself all the way at the end before Russ P.
@yarikoptic
Copy link
Contributor Author

done... wasn't sure where to place myself so 38cfd77

@oesteban
Copy link
Member

oesteban commented Apr 5, 2019 via email

@oesteban oesteban merged commit be411d7 into nipreps:master Apr 5, 2019
@oesteban
Copy link
Member

oesteban commented Apr 5, 2019

BTW, this made me think whether we should also delete other top-level files (e.g. pipeline_description.json, and this subject's html report and sub-XYZ folder)

@yarikoptic
Copy link
Contributor Author

well, if you keep thinking then may be you will even converge on my thinking that those files should not be just removed but loaded/compared to and if they are different - at least a warning or info message should be displayed like "you are now using different preprocessing settings from what you have used before - you might arrive to an incongruent collection of results (may be across or even within subjects!)" ;-) or that is not possible? ;-)

@oesteban
Copy link
Member

oesteban commented Apr 5, 2019

Well, that would apply to the boilerplate too, since it is dynamic and reflects the methodological choices.
(related to #1526).

oesteban added a commit to oesteban/fmriprep that referenced this pull request Jul 21, 2019
… fully run.

This PR addresses the memory peak caused by pandoc, running the process
at the very end of fMRIPrep.

We probably want to explicitly clear up as many objects as we can from
memory and force gc before the subprocess opens pandoc, but that's left
for a future PR.

This PR just closes nipreps#1709 with the delayed pandoc run. I'm not
implementing the flag to avoid the boilerplate generation because I'm
not sure we want to introduce an additioinal argument for this.

Coincidentally, it also closes nipreps#1667 - now the deletion of citation
files is done inside a try..catch where ``FileNotFoundError`` is
dismissed. That should avert the issue, while keeping the functionality
of nipreps#1567.
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