diffpy / diffpy.srmise

Other
0 stars 7 forks source link

refactor makeclusters to make it work #73

Closed stevenhua0320 closed 2 months ago

Ainamacar commented 2 months ago

(Edit: I originally posted this comment in Issues #64, but it's more appropriate here.)

I believe this commit will cause breaking changes. The main extraction loop of SrMise (located in extract_single() in peakextraction.py) currently relies on DataClusters following Python's iterator protocol. This commit is somewhere in between: DataClusters still defines __iter__ but since next() no longer exists it won't behave as intended when using an Iterator. (Looking at the docs, I also see that Python 3 iterators should implement a__next__ method instead of the next method used in Python 2.)

What is the desired scope of this refactor?

If it's mainly to make makeclusters() more easily understood, then this is on the right track, we just need to have a __next()__ method that behaves like next() did previously.

However, if it's so DataClusters no longer implements the iterator protocol (which I had assumed in my Issues #64 comments), then further changes are necessary. I regret not mentioning some of these originally, and not noticing others until now.

stevenhua0320 commented 2 months ago

Simon could you revert this merged commit right now? Since Luke said in the comment that this commit might cause breaking changes, I'm not sure right now if this would affect other files that depend on DataClusters class. For PR #75 it is just change the syntax from py2 to py3, so that is a safe refactor, and we should now use that.