diffpy / diffpy.srmise

Other
0 stars 7 forks source link

makeclusters() in DataClusters class undefined #64

Open stevenhua0320 opened 2 months ago

sbillinge commented 2 months ago

Please can you add more information on this issue so another person could read it and know what to do? Thx

stevenhua0320 commented 2 months ago

This function is a empty function and needs refactor. It needs to based on DataClusters object's data points and resolution to make clusters.

Ainamacar commented 2 months ago

This function only looks empty, it is actually foundational to the existing implementation. I'll summarize the current behavior and how it could be changed, if you wish to do so.

DataClusters is an iterable because it defines the __iter__ method. Python defines that the next() method returns the next member of an iterable. So when makeclusters() is run it is actually calling next() many times, and that is where the bulk of the work is done. That is why next() is never called explicitly -- it is done implicitly in makeclusters(). So the iteration is not over the clusters that have been found, but the process of clustering. I comment in the source that "This behavior could cause confusion and should perhaps be altered." Maybe that time is now?

If you wanted to change this behavior to make the clustering process more explicit (and thus easier to read/maintain), here is a possible approach.

sbillinge commented 2 months ago

Thanks for the feeback @Ainamacar .

Ainamacar commented 2 months ago

Edit: I moved a lengthy comment to PR #73, where it was more appropriate.