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: Add MultiObject, ensure/simplify_list; alias old names for 1.x compatibility #2517

Merged
merged 2 commits into from
Apr 24, 2018

Conversation

effigies
Copy link
Member

For the sake of smoothing the transition to 2.0, I propose that we introduce Input/OutputMultiObject in 1.x, but leave MultiPath traits in place. Downstream projects with dependency >=1.0.3 will be able to write interfaces that require one less change in the future.

@codecov-io
Copy link

codecov-io commented Mar 28, 2018

Codecov Report

Merging #2517 into master will increase coverage by <.01%.
The diff coverage is 63.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2517      +/-   ##
==========================================
+ Coverage   66.88%   66.89%   +<.01%     
==========================================
  Files         327      327              
  Lines       42491    42495       +4     
  Branches     5269     5269              
==========================================
+ Hits        28422    28426       +4     
  Misses      13367    13367              
  Partials      702      702
Flag Coverage Δ
#smoketests 50.86% <43.43%> (ø) ⬆️
#unittests 64.2% <55.88%> (-0.05%) ⬇️
Impacted Files Coverage Δ
nipype/interfaces/base/__init__.py 100% <ø> (ø) ⬆️
nipype/workflows/fmri/fsl/preprocess.py 84.97% <0%> (ø) ⬆️
nipype/workflows/smri/freesurfer/autorecon1.py 38.42% <0%> (ø) ⬆️
nipype/interfaces/spm/utils.py 60.78% <0%> (ø) ⬆️
nipype/pipeline/engine/nodes.py 83.94% <100%> (ø) ⬆️
nipype/testing/fixtures.py 98.66% <100%> (ø) ⬆️
nipype/interfaces/base/traits_extension.py 97.67% <100%> (+0.03%) ⬆️
nipype/algorithms/rapidart.py 64.6% <100%> (ø) ⬆️
nipype/utils/filemanip.py 78.06% <100%> (+0.1%) ⬆️
nipype/algorithms/misc.py 45.79% <100%> (ø) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc09e00...b33c3df. Read the comment docs.

@oesteban
Copy link
Contributor

Maybe some deprecation warning would be nice here? So to induce people moving into these new MultiObject?

@effigies
Copy link
Member Author

Well, we're not deprecating in 1.x, but FutureWarning might be appropriate.

On the other hand, I want to be careful about warnings that are targeted to developers, but will have a primary effect of annoying or scaring end users.

@satra
Copy link
Member

satra commented Apr 3, 2018

talk of other bad semantics, my other favorite one is filename_to_list. i think similar to scikit-learn it should be called ensure_list and perhaps change list_to_filename to simplify_list

let's keep warnings out, but update our docs where we reference these objects to the new ones.

@effigies
Copy link
Member Author

Input/OutputMultiPath and filename_to_list/list_to_filename don't appear in the docs, so there's not much left to do.

Incidentally, I couldn't find ensure_list in scikit-learn source. Do you have a reference? It would be nice to use consistent naming with other projects.

@satra
Copy link
Member

satra commented Apr 10, 2018

@effigies - scikit-learn has ensure_2d as a keyword in some of its functions.

but perhaps the closest is np.asarray and that made me think aslist might be a good alternative to ensure_list and shorter?

@effigies
Copy link
Member Author

Hmm. aslist feels more ambiguous to me than ensure_list. Pybids uses listify. We could do listify/unlistify, but I'm also fine with ensure/simplify.

@@ -573,13 +573,17 @@ def list_to_filename(filelist):
return filelist[0]


filename_to_list = ensure_list
list_to_filename = simplify_list
Copy link
Member

Choose a reason for hiding this comment

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

perhaps we generate warnings for these as well as multiobject to have people start using the new objects.

def filename_to_list(filelist):
    logger.warn('Please use `ensure_list` instead')
    return ensure_list(filelist)

Copy link
Member Author

Choose a reason for hiding this comment

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

From #2517 (comment):

let's keep warnings out, but update our docs where we reference these objects to the new ones.

I think before we start adding warnings for things we don't plan to drop support for until 2.0, we should try to come up with a coherent plan for warnings and deprecations. While I want to make new names available for people trying to future proof, I don't want to annoy users unduly.

I can open an issue on the subject, where we can try to map out a plan. Seem reasonable?

Copy link
Member

Choose a reason for hiding this comment

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

roger.

@satra
Copy link
Member

satra commented Apr 14, 2018

i would be ok with listify/unlistify if we wanted to conform with pybids - but fine with the current names as well.

@effigies effigies added this to the 1.0.3 milestone Apr 17, 2018
@effigies effigies changed the title ENH: Rename MultiPath -> MultiObject ENH: Add MultiObject, ensure/simplify_list; alias old names for 1.x compatibility Apr 24, 2018
@effigies effigies merged commit 5c3c413 into nipy:master Apr 24, 2018
@effigies effigies deleted the enh/multi_object branch April 24, 2018 19:13
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.

4 participants