neutrons / multiphonon

multiphonon (getDOS)
MIT License
2 stars 5 forks source link

porting to python 3 #130

Closed yxqd closed 3 years ago

coveralls commented 3 years ago

Coverage Status

Coverage increased (+0.03%) to 88.576% when pulling 69f2f5ee6f0c7018e2d318ef9249b439e234ed4c on 2to3 into 389ada4c38a1d2a65db12cc79ce9b844caecd8a2 on master.

granrothge commented 3 years ago

@yxqd How would you like me to review the code? Overall it looks pretty good. A couple of style notes. 1) In the following code snipet a python slight-of-hand is used by setting context and thus setting self. context in the next to last line. A comment should be given or you should explicitly set self.context.iqe_h5 etc.

def nextStep(self):
        import warnings
        warnings.simplefilter(action = "ignore", category = FutureWarning)
        from multiphonon.getdos import reduce2iqe
        context = self.context
        r2i = reduce2iqe(
            context.sample_nxs,
            context.Emin, context.Emax, context.dE,
            context.Qmin, context.Qmax, context.dQ,
            mt_nxs=context.mt_nxs,
            workdir=context.workdir,
        )
        for msg in r2i:print(msg)
        context.iqe_h5, context.mtiqe_h5, context.Qaxis, context.Eaxis = msg
        print("Done.")  

2) I would not change it now but in the future go away from the % syntax to format strings to the .format syntax. PEP has promised it won't go away soon, but we want to minimize that change when it comes.

yxqd commented 3 years ago

Thanks for the comments, @granrothge !