Some general feedback

[Qqwy]

Here are some notes, after having glanced over the library during breakfast.

• Calendrical:

  • The naming of days/1 for a function that returns the number of what day 'monday' is feels counter-intuitive to me. Maybe weekday_number/1 or something similar is better. Besides, the names of the weekdays (and even if there are seven days inside) depend on the actual calendar and language used, so maybe this should better be moved to another module (Such as Kday).
  • Probably, the datetime-geared functions should be removed. %DateTime{} stores the timezone offset on top of what NaiveDateTime does. At least Elixir's standard library takes the approach of having developers explicitly think about this timezone management (and thus always having the explicit NaiveDateTime -> DateTime conversion step).
  • For this library it is unfortunate that some of the RataDie-calculations inside Elixir's standard library are managed in private functions. Maybe an alternate idea would be to create a %RataDie{} calendar implementation (with trivial naive_datetime_to_rata_die and naive_datetime_from_rata_die-implementations).

• Calendrical.Math:

  • At least the angles_to_radians functionality is not required by any calendars, as far as I know (or is it? :grin: )
  • It is worth noting that Integer (courtesy of the Calendar additions with the Rata Die day fraction calculations) now contains a gcd/2.

• Calendrical.JulianDay:

  • line 58 contains a magic number. Probably better to create a constant for the middle of the day.
  • I don't actually think there is a reason to work with microsecond-precision day fractions here. You could just use the lowest fraction that allows you to express something. For noon, this would be {1, 2}.
  • Line 84 might convert the result to a float, which is probably not what you want.
  • Why is Calendrical.JulianDay not a Calendar behaviour implementation?

• Calendrical.Kday:

  • Seems fine, although it is been a while since I read the K-day part of the Calendrical Calculations paper. :sweat_smile:

Thank you for writing this! :heart:

[kipcole9]

@qqwy thank you very much. i am definitely standing on the shoulders of giants here.

Your feedback and insight are very helpful, i'll work on my next commit and see if i can converge on a happier path.

Regards, Kip

Sent from my iPhone

On 28 Jun 2017, at 2:49 PM, Qqwy notifications@github.com wrote:

Here are some notes, after having glanced over the library during breakfast.

Calendrical:

The naming of days/1 for a function that returns the number of what day 'monday' is feels counter-intuitive to me. Maybe weekday_number/1 or something similar is better. Besides, the names of the weekdays (and even if there are seven days inside) depend on the actual calendar and language used, so maybe this should better be moved to another module (Such as Kday). Probably, the datetime-geared functions should be removed. %DateTime{} stores the timezone offset on top of what NaiveDateTime does. At least Elixir's standard library takes the approach of having developers explicitly think about this timezone management (and thus always having the explicit NaiveDateTime -> DateTime conversion step). For this library it is unfortunate that some of the RataDie-calculations inside Elixir's standard library are managed in private functions. Maybe an alternate idea would be to create a %RataDie{} calendar implementation (with trivial naive_datetime_to_rata_die and naive_datetime_from_rata_die-implementations). Calendrical.Math:

At least the angles_to_radians functionality is not required by any calendars, as far as I know (or is it? 😁 ) It is worth noting that Integer (courtesy of the Calendar additions with the Rata Die day fraction calculations) now contains a gcd/2. Calendrical.JulianDay:

line 58 contains a magic number. Probably better to create a constant for the middle of the day. I don't actually think there is a reason to work with microsecond-precision day fractions here. You could just use the lowest fraction that allows you to express something. For noon, this would be {1, 2}. Line 84 might convert the result to a float, which is probably not what you want. Why is Calendrical.JulianDay not a Calendar behaviour implementation? Calendrical.Kday:

Seems fine, although it is been a while since I read the K-day part of the Calendrical Calculations paper. 😅 Thank you for writing this! ❤️

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[kipcole9]

Thanks for the generous feedback.

1. Have taken out the DateTime functions as you suggest - makes good sense to leave the reasoning to the programmer.

2. Renamed days/1 to day_number/1 and moved it into Calendrical.Kday. I realise that not all calendars have 7 days in a week (Balinese, Chinese, ..) but a lot do. I'm trying to strike a balance so that when I implement some of the additional calendars I don't have to duplicate this reasoning everywhere. But I can defer that until I actually need to do something.

3. The Calendrical.Math module is to support not only the Arithmetic calendars (which don't need angles_to_radians) and Astronomical calendars which do. Of course they need a whole lot of Astro calculations which I have been working on in the background. It makes my head spin but its great fun. And good catch on gcd, I'll use the Elixir one since Calendrical is dependent on Elixir 1.5 anyway.

I'll revisit Julian Day - a Calendrical behaviour makes sense unless you think its worth building our as a PR for the Calendar module.