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

MAINT: Deprecations #1243

Merged
merged 8 commits into from
Aug 6, 2023
Merged

MAINT: Deprecations #1243

merged 8 commits into from
Aug 6, 2023

Conversation

larsoner
Copy link
Contributor

  1. Recently for some reason in some joblib-parallel runs I've gotten:

    ../mne-python/mne/_freesurfer.py:89: in _reorient_image
        ornt = nib.orientations.axcodes2ornt(
    E   AttributeError: module 'nibabel' has no attribute 'orientations'
    

    Why this started happening just recently is a mystery to me, but this PR adds from . import orientations to nibabel/__init__.py which hopefully seems reasonable. (It has worked in MNE for at least a year, not sure why it just became an issue...)

  2. Deals with warnings of the form:

    ../nibabel/nibabel/nifti1.py:20: in <module>
        from numpy.compat.py3k import asstr
    ../virtualenvs/base/lib/python3.11/site-packages/numpy/compat/__init__.py:21: in <module>
        warnings.warn(
    E   DeprecationWarning: `np.compat`, which was used during the Python 2 to 3 transition, is deprecated since 1.26.0, and will be removed
    

@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (6fe3e6b) 92.16% compared to head (5bb4d4c) 92.16%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1243   +/-   ##
=======================================
  Coverage   92.16%   92.16%           
=======================================
  Files          98       98           
  Lines       12368    12370    +2     
  Branches     2540     2542    +2     
=======================================
+ Hits        11399    11401    +2     
  Misses        646      646           
  Partials      323      323           
Files Changed Coverage Δ
nibabel/__init__.py 100.00% <100.00%> (ø)
nibabel/nicom/utils.py 100.00% <100.00%> (ø)
nibabel/nifti1.py 92.75% <100.00%> (-0.01%) ⬇️
nibabel/streamlines/trk.py 93.98% <100.00%> (-0.02%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@effigies
Copy link
Member

How do you feel about trying to purge asstr() and asbytes() altogether? This is fine as a stop-gap if we just need to get it working, but I would like to purge them sooner or later.

Also unclear why nib.orientations stopped working, but yeah, it's fine to have without a special import.

@larsoner
Copy link
Contributor Author

larsoner commented Jul 18, 2023

How do you feel about trying to purge asstr() and asbytes() altogether? This is fine as a stop-gap if we just need to get it working, but I would like to purge them sooner or later.

asbytes is already gone (now I just do bytes(..., 'ascii')) and I could probably do something similar with asstr. But the advantage of asstr is that it has a conditional inside it. So if there are times that something could by bytes or str it keeps things DRY. But it is less explicit, so I could replace the 3 (I think?) uses to have conditionals in each case.

Comment on lines 803 to 805
if isinstance(s, bytes):
return s.decode('latin1')
return str(s)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

... in other words, unless in each instance that asstr is used we know something is always either bytes or str (in which case, why use this func at all???) then the places asstr is used we'll need to put this conditional in. But maybe that's cleaner... I'm happy either way!

@effigies
Copy link
Member

My hope is that we actually can know what types we're getting and whether or not to coerce them, rather than always having conditionals, but I haven't looked into them.

That's the task I'm asking if you want to undertake. No worries if not, and I'm fine with keeping asstr until somebody (perhaps even me) does feel the urge.

@larsoner
Copy link
Contributor Author

My hope is that we actually can know what types we're getting and whether or not to coerce them, rather than always having conditionals, but I haven't looked into them.

I took a mostly naive approach by decoding in all cases. Hopefully that works -- it did in limited testing locally -- but it probably depends on how good your test coverage is! If you want deeper investigation, nibabel/nicom/utils.py:find_private_section for example is beyond my investigative skill level given available time :) In which case I can revert the "always-decode" commit 28a9639 ...

Comment on lines +33 to +34
if isinstance(creator, bytes):
creator = creator.decode('latin-1')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one has both bytes and str tests in nicom/tests/test_utils.py so I assume it's necessary

@larsoner
Copy link
Contributor Author

Well it's green now so feel free to merge if you want. Safer to revert the last three commits but if you trust your tests you should be okay!

@effigies
Copy link
Member

effigies commented Aug 4, 2023

Pushed some minor changes directly. Should not affect tests, and will merge on pass.

Thanks for putting in the extra effort!

@effigies effigies merged commit 8fea2a8 into nipy:master Aug 6, 2023
43 checks passed
@larsoner larsoner deleted the deps branch August 7, 2023 04:42
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