search

NCAS-CMS / cfdm

A Python reference implementation of the CF data model
http://ncas-cms.github.io/cfdm
MIT License
28 stars 11 forks source link

Python 3.12 compatability #292

Closed davidhassell closed 5 months ago

davidhassell commented 6 months ago

Fixes #302

Needed for https://github.com/NCAS-CMS/cf-python/issues/770

davidhassell commented 6 months ago

Thanks, Sadie.

I've made a few other related changes, and all the unit tests now pass with no warnings (at 3.12, I haven't tested < 3.12), so I think it's time for review. I've also got a related cf-python PR which I'll put up soon.

man cfdump does not work (couldn't work out how to do that, back in the day), so rendering cfdump -h to look just like a man page was the fall back option.

sadielbartholomew commented 6 months ago

Thanks David. To check before I review, it seems this PR is only to cover to fix the case of a backslash character invalid escape sequence, as per the linked issue and your identification of the 3.12 update that led to it in https://github.com/NCAS-CMS/cf-python/issues/770#issuecomment-2098054536, is that true? Or have you checked the list of updates for 3.12 and come to the conclusion that everything is compatible (the PR name suggests that, perhaps)? The test suite passing should obviously be a good sign that we're all good for 3.12 but I wouldn't want to rely on it. So might be wise, if you haven't already, that we read through the list of 'What’s New In Python 3.12' and convince ourselves everything is compatible. Wouldn't have to be in this PR, necessarily.

davidhassell commented 6 months ago

Hi Sadie,

Sorry for not replying to this. I have looked through the 3.12 changes and nothing stood out as needing attention ... and the unit test pass, so I think (hope!) that this is all we need to do for 3.12. Thanks.

sadielbartholomew commented 5 months ago

Hi David. Oh fair. Feel free of course to from my pushed commit, just write over it or even reset back from it or revert it with git revert. It was useful as an illustration of what could work at least, even if a different approach is safer / more sensible :smile: But overall I would strongly prefer if we sort that DeprecationWarning here in this PR, which promises full Python 3.12 compatibility...

I think that it's fine[] for us to diverge from numpy's behaviour (i.e. so that int(f) is not always the same as int(f.array)), but I think we need return int(self.array[(0,)self.ndim] to do this.

I can check to see how numpy does it if useful, they obviously won't want a DeprecationWarning coming through but if that's what they are effectively doing, they'll have to handle it somehow...

davidhassell commented 5 months ago

self.array[(0,)*self.ndim] will always be a 0-dim numpy array, so there should be no more deprecation warnings (and we've got our out TypeError trap on the previous line.

sadielbartholomew commented 5 months ago

self.array[(0,)*self.ndim] will always be a 0-dim numpy array, so there should be no more deprecation warnings (and we've got our out TypeError trap on the previous line.

OK that makes sense. I am happy for you to implement that approach, or I can re-push that update in a new commit if you prefer.

sadielbartholomew commented 5 months ago

Hi @davidhassell, thought I'd bump this since it's been a month or so but this is ready to merge if you just add your adjustment as per https://github.com/NCAS-CMS/cfdm/pull/292#issuecomment-2109768078 as agreed. I imagine it fell off your radar. (The SyntaxWarnings are quite annoying so good to prevent them.)

davidhassell commented 5 months ago

After some Kafka-esque git related confusion, I have updated Data.__int__, and added unit test to boot.

sadielbartholomew commented 5 months ago

After some Kafka-esque git related confusion

Ah sorry, my pushing to your branch probably caused that difficulty, I now realise. In which case I should have insisted to push the change myself. Next time!

Perfect, I've sanity checked the new commits and they do what we wanted so please merge.