Small coding style improvements

kevindlewis23 commented 2 weeks ago

No functional changes made here, mainly just removing unnecessary pass statements and putting underscores to mark unused variables. There are a couple bugfixes as well

pep8speaks commented 2 weeks ago

Hello @kevindlewis23! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2024-07-02 18:28:29 UTC

donald-e-boyce commented 2 weeks ago

I would hesitate to remove the Null class. In the base config file class, where it is used, it sets the default get value to null, which is explicitly not None. I can imagine cases where using a default of None has a specific meaning. I don't have any in particular in mind, but I can imagine such a case.

Also, the person who wrote that was a very experienced python programmer who most certainly knew about how to use None. I think that was intentionally not None.

On Mon, Jul 1, 2024 at 10:22 AM Zack @.***> wrote:

@.**** requested changes on this pull request.

In hexrd/config/utils.py https://github.com/HEXRD/hexrd/pull/659#discussion_r1661098370:

@@ -8,14 +8,6 @@ "sfacmin", "sfacmax", "pintmin", "pintmax"] )

- -class Null():

The Null class is currently being used here https://github.com/HEXRD/hexrd/blob/d57ef03b96abb1c9536d41add40f84606c63dd49/hexrd/config/config.py#L6 and elsewhere in this script. If we're going to remove the class, we should replace instances of Null with the more correct and Pythonic None. Otherwise, this PR will cause failures.

In hexrd/findorientations.py https://github.com/HEXRD/hexrd/pull/659#discussion_r1661101004:

@@ -131,9 +125,12 @@ def generate_orientation_fibers(cfg, eta_ome): ome_c = eta_ome.omeEdges[0] + (0.5 + coms[i][ispot][0])del_ome eta_c = eta_ome.etaEdges[0] + (0.5 + coms[i][ispot][1])del_eta input_p.append(np.hstack([this_hkl, this_tth, eta_c, ome_c]))

pass

pass

pass

params = dict(

Is it necessary to move this?

In hexrd/fitting/fitpeak.py https://github.com/HEXRD/hexrd/pull/659#discussion_r1661104639:

@@ -58,15 +56,15 @@ def cnst_fit_obj(x, b): return np.ones_like(x)*b

-def cnst_fit_jac(x, b): +def cnst_fit_jac(x, _b):

This function doesn't use _b. I'm not sure that changing the variable name is the right choice - if we're going to change this, we should consider fixing the signature to not have a second argument at all. Though, it may affect downstream user workflows unexpectedly.

In hexrd/fitting/fitpeak.py https://github.com/HEXRD/hexrd/pull/659#discussion_r1661105315:
 return np.vstack([np.ones_like(x)]).T
def lin_fit_obj(x, m, b): return m*np.asarray(x) + b

-def lin_fit_jac(x, m, b): +def lin_fit_jac(x, _m, _b):

Same comment as prior - if _m and _b aren't being used by the function, we shouldn't have them.

In hexrd/fitting/fitpeak.py https://github.com/HEXRD/hexrd/pull/659#discussion_r1661107204:

@@ -77,7 +75,6 @@ def quad_fit_obj(x, a, b, c):

def quad_fit_jac(x, a, b, c): x = np.asarray(x)

return a*x*2 + bx + c

Are we sure this is the incorrect return? The return that's staying doesn't seem to include a, b, or c.

In hexrd/fitting/fitpeak.py https://github.com/HEXRD/hexrd/pull/659#discussion_r1661109994:

@@ -198,13 +195,13 @@ def fit_pk_parms_1d(p0, x, f, pktype='pvoigt'): weight = np.max(f)*10. # hard coded should be changed fitArgs = (x, f, pktype) if pktype == 'gaussian':

p, outflag = optimize.leastsq(

p, _outflag = optimize.leastsq(

Is outflag even used anywhere? If not, consider replacing all instances of it with .

In hexrd/fitting/grains.py https://github.com/HEXRD/hexrd/pull/659#discussion_r1661118813:

@@ -234,7 +233,7 @@ def objFuncFitGrain(gFit, gFull, gFlag, gHat_c = mutil.unitVector(np.dot(rMat_c.T, gVec_s))
     # !!!: check that this operates on UNWARPED xy
match_omes, calc_omes = matchOmegas(

_match_omes, calc_omes = matchOmegas(

If _matchomes is not being used, this should be replaced with . Though, it is a bit strange that matchOmegas would be called, and the _match_omes result would not be used. Something smells off in the workflow.

In hexrd/material/crystallography.py https://github.com/HEXRD/hexrd/pull/659#discussion_r1661120428:

@@ -1385,7 +1385,7 @@ def getHKLs(self, *hkl_ids, **kwargs): opts = dict(asStr=False, thisTTh=None, allHKLs=False) if len(kwargs) > 0:

Could even remove the if len(kwargs) > 0 check.

In hexrd/nf_config/experiment.py https://github.com/HEXRD/hexrd/pull/659#discussion_r1661123986:

@@ -33,33 +33,38 @@ def max_tth(self): def comp_thresh(self): key = 'experiment:comp_thresh'

No need to set a variable for key, this can be removed - just refer to the string directly.

In hexrd/nf_config/experiment.py https://github.com/HEXRD/hexrd/pull/659#discussion_r1661125272:

@@ -33,33 +33,38 @@ def max_tth(self): def comp_thresh(self): key = 'experiment:comp_thresh' temp = self._cfg.get(key, None)

{}.get(k) defaults to None already if the key is not found. This can be temp = self._cfg.get(key).

Or better yet, temp = self._cfg.get('experiment:comp_thresh')

In hexrd/nf_config/experiment.py https://github.com/HEXRD/hexrd/pull/659#discussion_r1661126528:

if temp is None:

return temp

elif np.logical_and(temp <= 1.0, temp > 0.0):

return temp

else:

raise RuntimeError('comp_thresh must be None or a number between 0 and 1')

if temp is not None and np.any(

np.logical_or(temp > 1.0, temp <= 0.0)

):

raise RuntimeError(

'comp_thresh must be None or a number between 0 and 1'

)

return temp

@property def chi2_thresh(self): key = 'experiment:chi2_thresh' temp = self._cfg.get(key, None)

Same comments as prior - remove unnecessary key variable, remove None argument from .get()

In hexrd/nf_config/experiment.py https://github.com/HEXRD/hexrd/pull/659#discussion_r1661127473:
 @property
def chi2_thresh(self): key = 'experiment:chi2_thresh' temp = self._cfg.get(key, None)

if temp is None:

return temp

elif np.logical_and(temp <= 1.0, temp > 0.0):

return temp

else:

raise RuntimeError('chi2_thresh must be None or a number between 0 and 1')

if temp is not None and np.any(

np.logical_or(temp > 1.0, temp <= 0.0)

):

raise RuntimeError(

'chi2_thresh must be None or a number between 0 and 1'

)

return temp
 @property
 def misorientation(self):
     key = self._cfg.get('experiment:misorientation:use_misorientation')
     if key is True:
if key is True is killing me...

In hexrd/projections/polar.py https://github.com/HEXRD/hexrd/pull/659#discussion_r1661130582:

@@ -251,7 +251,7 @@ def warp_image(self, image_dict, pad_with_nans=False, panel)
         xypts = np.nan*np.ones((len(gvec_angs), 2))
valid_xys, rmats_s, on_plane = _project_on_detector(*args,

valid_xys, _rmats_s, on_plane = _project_on_detector(*args,

If _rmatss is not used, this should be replaced with .

In hexrd/projections/spherical.py https://github.com/HEXRD/hexrd/pull/659#discussion_r1661131352:

@@ -105,7 +105,7 @@ def warp_eta_ome_map(self, eta_ome, map_ids=None, skip=10): op.flatten() ]).T

ppts, nmask = zproject_sph_angles(

ppts, _nmask = zproject_sph_angles(

If nmask is not used, this should be replaced with .

In hexrd/sampleOrientations/conversions.py https://github.com/HEXRD/hexrd/pull/659#discussion_r1661133572:

@@ -12,9 +12,7 @@

@numba_njit_if_available(cache=True, nogil=True) def getPyramid(xyz):

x = xyz[0]

y = xyz[1]

z = xyz[2]

x, y, z = xyz

Please include a check that len(xyz) is 3 and is an unpackable object. Otherwise, this will raise a ValueError.

In hexrd/nf_config/experiment.py https://github.com/HEXRD/hexrd/pull/659#discussion_r1661129289:
 @property
def chi2_thresh(self): key = 'experiment:chi2_thresh' temp = self._cfg.get(key, None)

if temp is None:

return temp

elif np.logical_and(temp <= 1.0, temp > 0.0):

return temp

else:

raise RuntimeError('chi2_thresh must be None or a number between 0 and 1')

if temp is not None and np.any(

np.logical_or(temp > 1.0, temp <= 0.0)

):

raise RuntimeError(

'chi2_thresh must be None or a number between 0 and 1'

)

return temp
 @property
 def misorientation(self):
     key = self._cfg.get('experiment:misorientation:use_misorientation')
     if key is True:
This should be typed to bool so that we can be sure if key: is safe to use instead. The danger is that it's a string, for example, and would evaluate to truthy. But we should just enforce that not to be possible.

In hexrd/nf_config/experiment.py https://github.com/HEXRD/hexrd/pull/659#discussion_r1661129699:
 @property
def chi2_thresh(self): key = 'experiment:chi2_thresh' temp = self._cfg.get(key, None)

if temp is None:

return temp

elif np.logical_and(temp <= 1.0, temp > 0.0):

return temp

else:

raise RuntimeError('chi2_thresh must be None or a number between 0 and 1')

if temp is not None and np.any(

np.logical_or(temp > 1.0, temp <= 0.0)

):

raise RuntimeError(

'chi2_thresh must be None or a number between 0 and 1'

)

return temp
 @property
 def misorientation(self):
     key = self._cfg.get('experiment:misorientation:use_misorientation')
     if key is True:
There's also no need to have the key variable here either.

— Reply to this email directly, view it on GitHub https://github.com/HEXRD/hexrd/pull/659#pullrequestreview-2151471216, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIWT6PDY4BC53QPSPEDZTLZKFQY7AVCNFSM6AAAAABKFTYLG6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNJRGQ3TCMRRGY . You are receiving this because you are subscribed to this thread.Message ID: @.***>

kevindlewis23 commented 2 weeks ago

Null change was reverted

ZackAttack614 commented 2 weeks ago

I would like to consider removing the class. It seems its only use is as a substitute for None. I'm not convinced that it's useful.

kevindlewis23 commented 2 weeks ago

I did try to replace it with None but the test cases threw errors because the comparisons ended up not working. I couldn't figure out why immediately, but I can try to fix it if you think it's worth it

donald-e-boyce commented 2 weeks ago

I would like to consider removing the class. It seems its only use is as a substitute for None. I'm not convinced that it's useful.

It's such a minor change, I don't think it's worth the effort. I think the choice to not use None was intentional.

psavery commented 2 weeks ago

Be sure to squash these commits, or cherry-pick the relevant ones.

HEXRD / hexrd

Small coding style improvements #659

Comment last updated at 2024-07-02 18:28:29 UTC

@.**** requested changes on this pull request.

Is it necessary to move this?

This function doesn't use _b. I'm not sure that changing the variable name is the right choice - if we're going to change this, we should consider fixing the signature to not have a second argument at all. Though, it may affect downstream user workflows unexpectedly.

Same comment as prior - if _m and _b aren't being used by the function, we shouldn't have them.

Are we sure this is the incorrect return? The return that's staying doesn't seem to include a, b, or c.

Is outflag even used anywhere? If not, consider replacing all instances of it with .

If _matchomes is not being used, this should be replaced with . Though, it is a bit strange that matchOmegas would be called, and the _match_omes result would not be used. Something smells off in the workflow.

Could even remove the if len(kwargs) > 0 check.

No need to set a variable for key, this can be removed - just refer to the string directly.

Or better yet, temp = self._cfg.get('experiment:comp_thresh')

Same comments as prior - remove unnecessary key variable, remove None argument from .get()

if key is True is killing me...

If _rmatss is not used, this should be replaced with .

If nmask is not used, this should be replaced with .

Please include a check that len(xyz) is 3 and is an unpackable object. Otherwise, this will raise a ValueError.

This should be typed to bool so that we can be sure if key: is safe to use instead. The danger is that it's a string, for example, and would evaluate to truthy. But we should just enforce that not to be possible.