Implement zmanim calculations #2

pinnymz · 2018-08-17T17:27:32Z

No description provided.

yeedle · 2018-08-19T02:42:55Z

Just a minor note on typehints. You're using numbers.Number, but PEP 484 claims this won't work, and says float should be used instead.

yeedle · 2018-08-19T03:09:48Z

    def elevation(self, elevation):
        if elevation is None:
            elevation = 0
        if elevation < 0:
            raise ValueError("elevation cannot be negative")
        self.__elevation = elevation

- source

You can simplify that first condition by taking advantage of None being falsy:

    def elevation(self, elevation):
        if elevation < 0:
            raise ValueError("elevation cannot be negative")
        self.__elevation = elevation or 0

pinnymz · 2018-08-19T03:31:00Z

@yeedle Re: #2 (comment)
isinstance(foo, float) will return false for an int. What would you expect to use here if not numbers.Number?

pinnymz · 2018-08-19T03:33:48Z

@yeedle Re: #2 (comment)
if elevation < 0 will fail if elevation is None, how to address this?

yeedle · 2018-08-19T03:46:56Z

re type checking: I was referring to the typehints/type annotations, but for isinstance, I guess Number works, or you can use isinstance(foo, (int, float))

yeedle · 2018-08-19T03:49:23Z

re elevation < 0, good catch. if elevation and elevation < 0 should remedy that, but I'm not sure it's better than an explicit check for None

leahein · 2018-08-19T16:22:36Z

With regards to the double leading underscores that are used:

From PEP-8
https://www.python.org/dev/peps/pep-0008/#designing-for-inheritance

Generally, double leading underscores should be used only to avoid name conflicts with attributes in classes designed to be subclassed.
Note: there is some controversy about the use of __names (see below).

Note 1: Note that only the simple class name is used in the mangled name, so if a subclass chooses both the same class name and attribute name, you can still get name collisions.

Note 2: Name mangling can make certain uses, such as debugging and __getattr__(), less convenient. However the name mangling algorithm is well documented and easy to perform manually.

Note 3: Not everyone likes name mangling. Try to balance the need to avoid accidental name clashes with potential use by advanced callers.

In general, the convention for private methods would be a single leading underscore. Unless this is truly a case where there's a specific need to avoid name clashes with subclasses.

https://stackoverflow.com/a/6930223

leahein · 2018-08-19T16:31:48Z

zmanim/astronomical_calendar.py

+        noon_hour = (self.temporal_hour(sunrise, sunset) / self.HOUR_MILLIS) * 6.0
+        return sunrise + timedelta(noon_hour / 24.0)
+
+    def __date_time_from_time_of_day(self, time_of_day: float, mode: str) -> datetime:


This returns an Optional[datetime], based on line 80

Also, since it appears that time_of_day accepts None, the arg type should be Optional[float] as well.

leahein · 2018-08-19T16:33:04Z

zmanim/astronomical_calendar.py

+    def utc_sea_level_sunset(self, zenith: float) -> float:
+        return self.astronomical_calculator.utc_sunset(self.__adjusted_date(), self.geo_location, zenith, adjust_for_elevation=False)
+
+    def temporal_hour(self, sunrise: datetime = __sentinel, sunset: datetime = __sentinel) -> float:


This returns an Optional[float], based on line 65

leahein · 2018-08-19T16:43:13Z

zmanim/astronomical_calendar.py

+        noon_hour = (self.temporal_hour(sunrise, sunset) / self.HOUR_MILLIS) * 6.0
+        return sunrise + timedelta(noon_hour / 24.0)
+
+    def __date_time_from_time_of_day(self, time_of_day: float, mode: str) -> datetime:


Also, since it appears that time_of_day accepts None, the arg type should be Optional[float] as well.

leahein · 2018-08-19T16:48:09Z

zmanim/astronomical_calendar.py

+
+    __sentinel=object()
+
+    def __init__(self, geo_location: GeoLocation = None, date: date = None, calculator: AstronomicalCalculations = None):


Since the arguments are being assigned None default values, the arg type for those is technically Optional[<type>].
This could be left as is, since mypy is smart enough to deduce the type is Optional here, but it's good to know that according to PEP 484, Type checkers should move towards requiring the optional type to be made explicit.
https://www.python.org/dev/peps/pep-0484/#union-types

might be getting nitpicky on the types -- mypy can be configured to be more or less strict, so really just depends.

don't think being careful about types is being nit-picky. Modules with good typehints are tremendously valueable, especially when you use them with a good IDE like PyCharm, etc.

I am using PyCharm and it indicates that it sees the arg type as Optional, return types as well. I'll update shortly.

To enforce the explicit optional, you can run mypy with the --no-implicit-optional flag.

…in favor of float

leviadler · 2018-08-22T18:27:41Z

Imports should be ordered by builtins, third party then local https://www.python.org/dev/peps/pep-0008/#imports
Also a good idea to alpha order them in each section but there is no official rule about that. Pycharm can do this for you with "Optimize imports".

pinnymz · 2018-08-24T15:22:45Z

@leviadler re: #2 (comment) thanks, nifty feature!

Pinny Markowitz added 4 commits August 9, 2018 14:49

functional GeoLocation, needs some cleanup

b19baae

first pass at zmanim calculations port

5c9861a

adding some file structures

4256b9d

JewishDate implementation

c5685e2

leahein reviewed Aug 19, 2018

View reviewed changes

explicit Optionals, single-underscore private methods, remove Number …

a98b6b9

…in favor of float

Pinny Markowitz added 2 commits August 24, 2018 11:15

switch to tz from pytz, implement jewish calendar calculations

5cc3be9

optimize imports

17a2719

pinnymz merged commit 7df0a8d into master Aug 24, 2018

pinnymz deleted the implement_zmanim_calculations branch August 24, 2018 15:27


		__sentinel=object()

		def __init__(self, geo_location: GeoLocation = None, date: date = None, calculator: AstronomicalCalculations = None):

Implement zmanim calculations #2

Implement zmanim calculations #2

Uh oh!

Conversation

pinnymz commented Aug 17, 2018

Uh oh!

yeedle commented Aug 19, 2018

Uh oh!

yeedle commented Aug 19, 2018

Uh oh!

pinnymz commented Aug 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pinnymz commented Aug 19, 2018

Uh oh!

yeedle commented Aug 19, 2018

Uh oh!

yeedle commented Aug 19, 2018

Uh oh!

leahein commented Aug 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pinnymz Aug 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leviadler commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pinnymz commented Aug 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pinnymz commented Aug 19, 2018 •

edited

Loading

leahein commented Aug 19, 2018 •

edited

Loading

pinnymz Aug 19, 2018 •

edited

Loading

leviadler commented Aug 22, 2018 •

edited

Loading