Deprecate Constant class #223

micahkim23 · 2017-10-30T00:19:47Z

Closes #50

spencerkclark

Thanks for starting on this @micahkim23! I have a couple initial comments on these changes.

It would be good to get @spencerahill's opinion on this too, but I think it might be a good idea to keep a module called constants.py to house just the minimum remaining necessary constants (e.g. GRAV and R_E). One could import them from calc.py, model.py, and vertcoord.py, and it would prevent the need to define GRAV twice.

spencerkclark · 2017-10-30T01:05:23Z

aospy/calc.py

 from . import internal_names
 from . import utils
 from .var import Var


 logging.basicConfig(level=logging.INFO)

+GRAV = (


@micahkim23 @spencerahill would we like to make these DataArrays rather than tuples (as suggested in #50 (comment)), or is it not worth it? This would give the benefit of named attributes, and remove the need to select the first element when doing mathematical operations.

@spencerkclark I think the DataArrays are better than tuples. Later on, having tuples means that we will have to keep track of which indices we're selecting from the tuples which is cumbersome.

+1 on DataArrays. I had been thinking just floats but DataArrays much better.

micahkim23 · 2017-10-30T01:40:58Z

I think it would be a good idea to have a constants.py file. In the future, we may need more constants and we don't want those to be defined all over the place.

spencerahill · 2017-10-30T02:57:36Z

+1 on a constants module, but I'm leaning towards adding a leading underscore, i.e. _constants.py, to signify that it's not intended as public API.

Although, especially now that some of my work involves atmospheres of other planets, it occurs to me that we should eventually be more flexible with the values of these constants. But I think we can come back to that later as its own issue.

spencerahill · 2017-10-30T02:58:41Z

aospy/model.py

 from . import internal_names
 from . import utils

+R_E = (


Let's rename this RADIUS_EARTH to be more explicit

spencerahill · 2017-10-30T02:59:28Z

aospy/calc.py

 from . import internal_names
 from . import utils
 from .var import Var


 logging.basicConfig(level=logging.INFO)

+GRAV = (


Let's rename GRAV_EARTH to be more explicit, c.f. my comments on R_E and about earth v. other planetary atmospheres.

spencerahill · 2017-10-30T03:00:12Z

docs/whats-new.rst

@@ -12,8 +12,8 @@ Breaking Changes
 ~~~~~~~~~~~~~~~~

 - Deprecate ``Units`` class, so now the ``units`` attribute of the
-  ``Var`` class is a string. (fixes :issue:`50` via :pull:`222`).
-  By `Micah Kim <https://github.com/micahkim23>`_.
+  ``Var`` class is a string. Deprecate ``Constant`` class. (fixes :issue:`50`


Make this a separate bullet point from the previous one about Units.

spencerahill · 2017-10-30T03:01:25Z

LGTM besides a few minor comments I made above. Thanks @micahkim23 for the PR and @spencerkclark for the review.

spencerkclark · 2017-10-30T11:58:52Z

+1 for the the leading underscore for the _constants.py module.

Although, especially now that some of my work involves atmospheres of other planets, it occurs to me that we should eventually be more flexible with the values of these constants. But I think we can come back to that later as its own issue.

It feels most natural for these constants to be settable attributes of Run objects, but I agree this would require a bit of discussion to figure out how it should be done best.

spencerkclark

@micahkim23 I added a few more minor comments, but this is looking good!

spencerkclark · 2017-11-01T17:21:13Z

aospy/calc.py

@@ -470,7 +468,7 @@ def _compute_full_ts(self, data, monthly_mean=False, zonal_asym=False):
                                                 self.end_date)
            )
            if self.dtype_out_vert == 'vert_av':
-                full_ts *= (grav.value / self._to_desired_dates(self._ps_data))
+                full_ts *= (GRAV_EARTH.data / self._to_desired_dates(self._ps_data))


Here and elsewhere the constants are used, we should not need to use the data attribute. In other words I think the following should be OK:

full_ts *= (GRAV_EARTH / self._to_desired_dates(self._ps_data))

spencerkclark · 2017-11-01T17:22:39Z

docs/api.rst

@@ -192,20 +192,10 @@ automate
 Constants


I'll let @spencerahill weigh in, but I think since we are denoting _constants.py as private API now, we should probably delete this section from the documentation entirely.

spencerkclark · 2017-11-01T17:30:24Z

docs/whats-new.rst

@@ -11,6 +11,9 @@ v0.3 (unreleased)
 Breaking Changes
 ~~~~~~~~~~~~~~~~

+- Deprecate ``Constant`` class, so now we have a ``_constants.py`` file
+  containing commonly used constants. (fixes :issue:`50` via :pull:`223`).


nit: let's try and be a little more specific, something along the lines of "... so now we have a _constants.py module containing only the physical constants required by aospy, stored as scalar DataArrays."

Re-word as "Deprecate Constant class and constants.py module. Physical constants used internally by aospy are now stored in _constants.py (fixes :issue:50 via :pull:223)."

spencerkclark · 2017-11-01T17:35:40Z

aospy/_constants.py

@@ -0,0 +1,14 @@
+"""commonly used physical constants"""


nit: let's say "Physical constants used in aospy", because this is by no means an exhaustive list of commonly used physical constants :)

spencerkclark · 2017-11-01T17:54:32Z

@micahkim23 quick procedural question for you -- are you always squashing your commits before you push new changes? The reason I ask is that I am not notified via email when you make updates (otherwise I would have provided my comments sooner!). GitHub has a really nice "Squash and merge" option for PR's, so we can easily handle cleaning up the commits/commit messages upon merging. This way you can just push your newest sets of commits as you go (not worrying about cluttering up the commit history), and @spencerahill and I will be notified immediately.

micahkim23 · 2017-11-01T17:57:30Z

yeah, I've been squashing my commits before pushing new changes. I won't do that anymore, so that you guys can get notified sooner. Thanks for the heads up.

spencerkclark

Nice work @micahkim23. The build failure for Python 3.4 on Windows looks unrelated.

spencerahill · 2017-11-03T15:13:19Z

In it goes! Thanks @micahkim23 for the code and @spencerkclark for the review.

As I mentioned to @micahkim23 offline, this may not seem like all that important, if nobody is using this functionality much anyways. But in fact cleanup like this is really useful, as it helps us have a more streamlined codebase and enables us to think more clearly about the code that remains.

micahkim23 force-pushed the refactor branch from faf8249 to 60c10cf Compare October 30, 2017 00:21

micahkim23 changed the title ~~Deprecate Constants class~~ Deprecate Constant class Oct 30, 2017

spencerkclark reviewed Oct 30, 2017

View reviewed changes

spencerahill reviewed Oct 30, 2017

View reviewed changes

micahkim23 force-pushed the refactor branch 4 times, most recently from 1365702 to 7e2eecd Compare October 31, 2017 02:40

Deprecate Constant class

54d3b86

micahkim23 force-pushed the refactor branch from 7e2eecd to 54d3b86 Compare October 31, 2017 02:43

spencerkclark reviewed Nov 1, 2017

View reviewed changes

fixed naming issues

2fdbb7b

spencerkclark approved these changes Nov 3, 2017

View reviewed changes

spencerkclark mentioned this pull request Nov 3, 2017

Allow for user-configurable physical constants #225

Open

spencerahill merged commit f2efeaa into spencerahill:develop Nov 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate Constant class #223

Deprecate Constant class #223

micahkim23 commented Oct 30, 2017

spencerkclark left a comment

spencerkclark Oct 30, 2017

micahkim23 Oct 30, 2017

spencerahill Oct 30, 2017

micahkim23 commented Oct 30, 2017

spencerahill commented Oct 30, 2017

spencerahill Oct 30, 2017

spencerahill Oct 30, 2017

spencerahill Oct 30, 2017

spencerahill commented Oct 30, 2017

spencerkclark commented Oct 30, 2017 •

edited

Loading

spencerkclark left a comment

spencerkclark Nov 1, 2017

spencerkclark Nov 1, 2017

spencerahill Nov 1, 2017

spencerkclark Nov 1, 2017 •

edited

Loading

spencerahill Nov 1, 2017

spencerkclark Nov 1, 2017

spencerkclark commented Nov 1, 2017

micahkim23 commented Nov 1, 2017

spencerkclark left a comment

spencerahill commented Nov 3, 2017

Deprecate Constant class #223

Deprecate Constant class #223

Conversation

micahkim23 commented Oct 30, 2017

spencerkclark 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

micahkim23 commented Oct 30, 2017

spencerahill commented Oct 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spencerahill commented Oct 30, 2017

spencerkclark commented Oct 30, 2017 • edited Loading

spencerkclark 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

spencerkclark Nov 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spencerkclark commented Nov 1, 2017

micahkim23 commented Nov 1, 2017

spencerkclark left a comment

Choose a reason for hiding this comment

spencerahill commented Nov 3, 2017

spencerkclark commented Oct 30, 2017 •

edited

Loading

spencerkclark Nov 1, 2017 •

edited

Loading