pep8 compliance in pgamit classes #114

pdsmith90 · 2024-10-14T02:17:39Z

Formatting edits on a few things inside pgamit folder. Mostly shortening lines and cleaning spaces.

espg

@pdsmith90 thanks for starting on these fixes! I left a few minor suggestions and comments, but this looks quite close to being ready to merge

espg · 2024-10-14T12:11:52Z

pgamit/pyArchiveStruct.py

+        If only record is provided, only insert into db.
+        If only rinexobj is provided, then RinexRecord of rinexobj
+            is used for the insert.
+        If both are given, then RinexRecord overrides the passed record.
+
        :param record: a RinexRecord dictionary to make the insert to the db
        :param rinexobj: the pyRinex object containing the file being processed
        :param rnxaction: accion to perform to rinexobj.
-        :return: True if insertion was successful. False if no insertion was done.
+        :return: True if insertion was successful.
+                 False if no insertion was done.


@pdsmith90 @demiangomez we may want to adopt the numpydoc conventions for parameter and return definitions moving forward...

espg · 2024-10-14T12:17:11Z

pgamit/pyArchiveStruct.py

+            raise ValueError(('insert_rinex exception: both record and '
+                              'rinexobj cannot be None.'))


@demiangomez do we have a general preference between defaulting to implicit line joins vs triple quotes when dealing with long strings? For longer multiline statements I tend to think that triple quotes are easier to automate formatting of within the editor, but either convention is fine, or we can use both depending on situation without a default.

espg · 2024-10-14T12:30:09Z

pgamit/pyDate.py

+        return int(sdate[0]), int(sdate[1]), int(sdate[2]), int(sdate[3]), \
+            int(sdate[4])


this seems a bit awkward-- does the sdate object support slicing? Alternatively, parens line continuation will also work here

could we use
return tuple(int(sdate[i]) for i in range(5))

espg · 2024-10-14T12:44:48Z

pgamit/pyETM.py

+                    hl[t > self.date.fyear] = \
+                        np.log10(1. + (t[t > self.date.fyear] -
+                                 self.date.fyear) / r)


this is getting hard to read for me-- what about something like below, which is the same number of lines but splits on the parenthesis subtraction continuation:

varname = np.log10(1. + (t[t > self.date.fyear] - self.date.fyear) / r) hl[t > self.date.fyear] = varname

(deferring to @demiangomez on what an appropriate name for varname would be here)

I was trying to avoid using extra variables here, but I agree it is easier to read

espg · 2024-10-14T12:56:37Z

pgamit/pyETM.py

+    a = np.sin(dlat / 2) ** 2 + np.cos(lat1) * \
+        np.cos(lat2) * np.sin(dlon / 2) ** 2


preferred way of wrapping long lines is by using Python’s implied line continuation inside parentheses, brackets and braces, with operands and operators proximate-- i.e.:

a = (np.sin(dlat / 2) ** 2 + np.cos(lat1) * np.cos(lat2) * np.sin(dlon / 2) ** 2)

espg · 2024-10-14T13:18:48Z

pgamit/pyETM.py

+                source = self.soln.stack_name.upper() + \
+                    ' with ETM solution: filtered'


use parens continuation

espg · 2024-10-14T13:19:02Z

pgamit/pyETM.py

+            source = 'No ' + self.soln.stack_name.upper() + \
+                ' solution, no ETM: mean coordinate'


use parens continuation

espg · 2024-10-14T13:23:40Z

pgamit/pyETM.py

+        sig = np.sqrt(np.square(sig) + np.square(np.array(
+            [[sigma_h], [sigma_h], [sigma_v]])))


@pdsmith90 a bit more readable if each term being added is kept grouped together with the operand:

sig = np.sqrt(np.square(sig) + np.square(np.array([[sigma_h], [sigma_h], [sigma_v]])))

espg · 2024-10-14T13:41:23Z

pgamit/pyETM.py

-        # will be much larger than [1]'s `eps(mineig)`, since `mineig` is usually on
-        # the order of 1e-16, and `eps(1e-16)` is on the order of 1e-34, whereas
-        # `spacing` will, for Gaussian random matrixes of small dimension, be on
-        # othe order of 1e-16. In practice, both ways converge, as the unit test


@demiangomez not a comment on @pdsmith90 pep8 fixes, but a comment on the numerics-- numpy cholesky decomposition will accept any valid positive semi-definite input, and is much faster than the svd that is being used here. If you want / need to still output decomposition values for matrices which aren't positive semi-definite, I believe that you can use multivariate_normal, which will default to cholesky if possible (for speed), but then fall back to svd if cholesky raises a RuntimeWarning: covariance is not positive-semidefinite. warning during runtime.

Might speed up your runtime, but also not sure how much this code is dominating runtime for you at present.

espg · 2024-10-14T13:43:01Z

pgamit/pyETM.py

-            # self.polyhedrons = cnn.query_float('SELECT "X", "Y", "Z", "Year", "DOY" FROM gamit_soln '
-            #                                   'WHERE "Project" = \'%s\' AND "NetworkCode" = \'%s\' AND '
-            #                                   '"StationCode" = \'%s\' '
-            #                                   'ORDER BY "Year", "DOY", "NetworkCode", "StationCode"'
-            #                                   % (project, NetworkCode, StationCode))
-
-            # self.gamit_soln = GamitSoln(cnn, self.polyhedrons, NetworkCode, StationCode, project)
+            # self.polyhedrons = cnn.query_float(
+            #   'SELECT "X", "Y", "Z", "Year", "DOY" FROM gamit_soln '
+            #   'WHERE "Project" = \'%s\' AND "NetworkCode" = \'%s\' AND '
+            #   '"StationCode" = \'%s\' '
+            #   'ORDER BY "Year", "DOY", "NetworkCode", "StationCode"'
+            #   % (project, NetworkCode, StationCode))
+
+            # self.gamit_soln = GamitSoln(
+            #   cnn, self.polyhedrons, NetworkCode, StationCode, project)


@demiangomez can this be deleted and left in the git history?

pdsmith90 · 2024-10-14T14:46:11Z

Thanks for looking over the changes. If we want to use parenthesis continuation for strings, I will be sure to use that in the future.

espg · 2024-10-14T15:41:56Z

pgamit/pyETM.py

@@ -183,28 +184,32 @@ def __init__(self, m_type, **kwargs):
                elif isinstance(arg, float):
                    self.relaxation = np.array(arg)
                else:
-                    raise pyETMException_Model('\'relaxation\' must be list, numpy.ndarray, or float')
+                    raise pyETMException_Model(
+                        '\'relaxation\' must be list, numpy.ndarray, or float')


...I think from here to line 224, we probably don't need the python string escape characters (\) that are used. If we want quoted quotes, we can just switch from double to single without breaking the string:

"'relaxation' must be list, numpy.ndarray, or float")

espg · 2024-10-14T15:56:06Z

Thanks for looking over the changes. If we want to use parenthesis continuation for strings, I will be sure to use that in the future.

Yeah, of course. I do think that implicit continuation inside of [], {}, and ()'s is basically always preferred unless doing something with assert or with statements that doesn't allow doing implicit continuation.

espg · 2024-10-17T20:23:45Z

@pdsmith90 you'll need to update your master branch and rebase this branch before it can be merged

pdsmith90 added 7 commits October 10, 2024 14:15

pip8 compliance in dbConnection_old.py

76dc397

pip8 compliance in dbConnection.py

90c9c9e

pep8 compliance in pyArchiveStruct

dfad6fb

pep8 compliance pyBunch

e79c581

pep8 pyarchivestruct

619a39c

pep8 pydate

d89fcaa

pep8 pyetm part 1

681f5a7

espg requested changes Oct 14, 2024

View reviewed changes

espg reviewed Oct 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pep8 compliance in pgamit classes #114

pep8 compliance in pgamit classes #114

pdsmith90 commented Oct 14, 2024 •

edited

Loading

espg left a comment

espg Oct 14, 2024

espg Oct 14, 2024

espg Oct 14, 2024

pdsmith90 Oct 14, 2024 •

edited

Loading

espg Oct 14, 2024

pdsmith90 Oct 14, 2024

espg Oct 14, 2024

espg Oct 14, 2024

espg Oct 14, 2024

espg Oct 14, 2024

espg Oct 14, 2024

espg Oct 14, 2024

pdsmith90 commented Oct 14, 2024

espg Oct 14, 2024

espg commented Oct 14, 2024

espg commented Oct 17, 2024

		raise ValueError(('insert_rinex exception: both record and '
		'rinexobj cannot be None.'))

		return int(sdate[0]), int(sdate[1]), int(sdate[2]), int(sdate[3]), \
		int(sdate[4])

		a = np.sin(dlat / 2) ** 2 + np.cos(lat1) * \
		np.cos(lat2) * np.sin(dlon / 2) ** 2

		source = self.soln.stack_name.upper() + \
		' with ETM solution: filtered'

		source = 'No ' + self.soln.stack_name.upper() + \
		' solution, no ETM: mean coordinate'

		sig = np.sqrt(np.square(sig) + np.square(np.array(
		[[sigma_h], [sigma_h], [sigma_v]])))

pep8 compliance in pgamit classes #114

Are you sure you want to change the base?

pep8 compliance in pgamit classes #114

Conversation

pdsmith90 commented Oct 14, 2024 • edited Loading

espg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdsmith90 Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdsmith90 commented Oct 14, 2024

Choose a reason for hiding this comment

espg commented Oct 14, 2024

espg commented Oct 17, 2024

pdsmith90 commented Oct 14, 2024 •

edited

Loading

pdsmith90 Oct 14, 2024 •

edited

Loading