Small coding style improvements

[kevindlewis23]

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]

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! 🍻

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

[kevindlewis23]

Actual functionality change here, I assume the previous one was a bug

[ZackAttack614]

What is quad_fit_jac meant to do? If the removed return statement was incorrect, then does that mean the function doesn't need a, b, or c as arguments?

[ZackAttack614]

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.

[kevindlewis23]

I believe these functions are used by the optimizer, and this is the interface the optimizer uses

[ZackAttack614]

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

[kevindlewis23]

Same as above

[ZackAttack614]

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

[kevindlewis23]

Not sure, but it is actually a matrix which seems to make more sense. Doesn't affect the test cases which is unfortunate, so I couldn't tell.

[donald-e-boyce]

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 <#659 (comment)>: > @@ -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 <#659 (comment)>: > @@ -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 <#659 (comment)>: > @@ -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 <#659 (comment)>: > 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 <#659 (comment)>: > @@ -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 + b*x + 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 <#659 (comment)>: > @@ -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 <#659 (comment)>: > @@ -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 _match_omes 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 <#659 (comment)>: > @@ -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 <#659 (comment)>: > @@ -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 <#659 (comment)>: > @@ -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 <#659 (comment)>: > - 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 <#659 (comment)>: > @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 <#659 (comment)>: > @@ -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 _rmats_s is not used, this should be replaced with _. ------------------------------ In hexrd/projections/spherical.py <#659 (comment)>: > @@ -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 <#659 (comment)>: > @@ -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 <#659 (comment)>: > @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 <#659 (comment)>: > @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 <#659 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIWT6PDY4BC53QPSPEDZTLZKFQY7AVCNFSM6AAAAABKFTYLG6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNJRGQ3TCMRRGY> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[kevindlewis23]

Null change was reverted

[ZackAttack614]

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]

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

[kevindlewis23]

Accidental review, just supposed to be comments

[kevindlewis23]

Not sure, but it is actually a matrix which seems to make more sense. Doesn't affect the test cases which is unfortunate, so I couldn't tell.

[donald-e-boyce]

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]

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

[ZackAttack614]

What is quad_fit_jac meant to do? If the removed return statement was incorrect, then does that mean the function doesn't need a, b, or c as arguments?

[kevindlewis23]

I don't feel like resolving these merge conflicts