[feat] Adding an option to handle the polar night and midnight sun cases #30

korbav · 2020-12-14T09:49:29Z

In order to handle the particular cases of Midnight Sun & Polar Night days in the concerned areas, this PR adds a parameter polarNightMidnightSunResolver to the calculation parameters to let the user force the resolution of the prayers times normally not computable by choosing between one of thes 3 options described below.

Issue reference : #29

PolarNightMidnightSunResolver.Unresolved (default)
Will let the invalid prayer times as they are.
PolarNightMidnightSunResolver.AqrabBalad
Will determine the closest location that can resolve the prayers times and use it for the prayer times normally not defined.
Note: The resolution consists on increasing the latitude (if negative) or decreasing it (if positive) towards the latitude of the Equator (0 degree), by a 0.5 degree step.
The algorithm will stop if the latitude has been brought down to absolute 65 degrees and the resolution is still impossible.
Indeed, based on Wiki Midnight Sun, the phenomenon should not occur for the latitudes between ]-65.44°; 65.44°[
PolarNightMidnightSunResolver.AqrabYaum
Will determine the closest day that can resolve the prayers times and use it for the prayer times normally not defined.
Note: The resolution consists on adding a day before and after until we find a valid day, the algorithm stops after having tried 6 months forward and backward.

src/PolarNightMidnightSunResolver.js

src/CalculationParameters.js

src/PrayerTimes.js

z3bi

JazakAllahu khairun for this PR. I think with a few adjustments this will be a great addition to the library.

z3bi · 2020-12-14T14:24:19Z

I would recommend an approach where after these lines

dhuhrTime = new TimeComponents(solarTime.transit).utcDate(date.getFullYear(), date.getMonth(), date.getDate());
sunriseTime = new TimeComponents(solarTime.sunrise).utcDate(date.getFullYear(), date.getMonth(), date.getDate());
var sunsetTime = new TimeComponents(solarTime.sunset).utcDate(date.getFullYear(), date.getMonth(), date.getDate());

we do something like

if (sunriseTime.isInvalid || sunsetTime.isInvalid) {
   var resolved = polarCircleResolvedValues(date, coordinates)
   this.coordinates = resolved.coordinates;
   this.date = resolved.date;
   solarTime = resolved.solarTime;

    // get new times with resolved values
    dhuhrTime = new TimeComponents(solarTime.transit).utcDate(date.getFullYear(), date.getMonth(), date.getDate());
    sunriseTime = new TimeComponents(solarTime.sunrise).utcDate(date.getFullYear(), date.getMonth(), date.getDate());
    sunsetTime = new TimeComponents(solarTime.sunset).utcDate(date.getFullYear(), date.getMonth(), date.getDate());
}

and this way the resolution code is only calculating SolarTime instead of the full prayer time object which takes a lot of other factors into account.

korbav · 2020-12-14T18:04:50Z

I would recommend an approach where after these lines

dhuhrTime = new TimeComponents(solarTime.transit).utcDate(date.getFullYear(), date.getMonth(), date.getDate());
sunriseTime = new TimeComponents(solarTime.sunrise).utcDate(date.getFullYear(), date.getMonth(), date.getDate());
var sunsetTime = new TimeComponents(solarTime.sunset).utcDate(date.getFullYear(), date.getMonth(), date.getDate());

we do something like

if (sunriseTime.isInvalid || sunsetTime.isInvalid) {
   var resolved = polarCircleResolvedValues(date, coordinates)
   this.coordinates = resolved.coordinates;
   this.date = resolved.date;
   solarTime = resolved.solarTime;

    // get new times with resolved values
    dhuhrTime = new TimeComponents(solarTime.transit).utcDate(date.getFullYear(), date.getMonth(), date.getDate());
    sunriseTime = new TimeComponents(solarTime.sunrise).utcDate(date.getFullYear(), date.getMonth(), date.getDate());
    sunsetTime = new TimeComponents(solarTime.sunset).utcDate(date.getFullYear(), date.getMonth(), date.getDate());
}

and this way the resolution code is only calculating SolarTime instead of the full prayer time object which takes a lot of other factors into account.

This is definitely a great enhancement, I'm testing it but, it appears that even when the solar time is calculated correctly (which means !isNaN(solarTime.sunrise) && !isNaN(solarTime.sunset)), the following computation of fajr & isha prayer times can still be invalid.
To follow this approach, which I thing is the way to go, I believe there's another factor to take care of but I'm not able to determine which one. Indeed, despite of the solar time being valid, solarTime.hourAngle(-1 * calculationParameters.fajrAngle, false) can still be NaN.
The natural way I would handle that would be to pre-compute fajr time and to require its validity along side with the solarTime.
But the big drawback is that computing fajr time implies to compute almost everything else and I feel that doing so would lead us right back to our previous concern.
I would be glad to get your input on that.

z3bi · 2020-12-14T18:36:39Z

@korbav that should actually be ok. If you look further down, we apply the high latitude rule to find isha and fajr times if we are in a situation that has no twilight.

if (ishaTime == null || isNaN(ishaTime.getTime()) || safeIsha < ishaTime) {
    ishaTime = safeIsha;
}

korbav · 2020-12-14T21:53:57Z

@korbav that should actually be ok. If you look further down, we apply the high latitude rule to find isha and fajr times if we are in a situation that has no twilight.
if (ishaTime == null || isNaN(ishaTime.getTime()) || safeIsha < ishaTime) {
    ishaTime = safeIsha;
}

It seems that the problem was coming from tomorrowSolarTime, safeFajr method uses it indirectly.
I added a "resolver" the same way we did for today. That makes sense and seems to solve the problem.

src/CalculationParameters.js

z3bi · 2020-12-15T02:57:10Z

src/PolarCircleResolution.js

+      }
+      break;
+    }
+    default:


the default case should return the original values

src/PolarCircleResolution.js

z3bi · 2020-12-15T03:05:10Z

src/PrayerTimes.js

+          (isNaN(tomorrowSolarTime.sunrise))
+          && polarCircleResolver !== PolarCircleResolution.unresolved
+        ) {
+            const resolved = polarCircleResolvedValues(polarCircleResolver, tomorrow, coordinates);


This concerns me slightly, as tomorrowSunrise needs to actually be one day later than the sunrise value. It's used for some high latitude adjustments like 1/7 of the night calculations. With the aqrab yaum calculation it might go back to the same day that the original resolver arrived at.

Perhaps we should move the first invalid check down here and then check if either sunrise, sunset or tomorrowSunrise are invalid. Then we can have the resolver find values that will allow the date and the following date to have valid sunrise and sunset times.

If my understanding is correct, we must ensure that the tomorrow's solar time will precisely correspond to the solar time of the day after the one we resolved for the today's solar time.
Thus, what we need in the resolution process, is to ensure that for the resolved today's solar time, the solar time of the day after is also valid. I made the changes correspondingly to that understanding.

z3bi · 2020-12-16T02:51:59Z

src/PrayerTimes.js

+            dhuhrTime = new TimeComponents(solarTime.transit).utcDate(...dateComponents);
+            sunriseTime = new TimeComponents(solarTime.sunrise).utcDate(...dateComponents);
+            sunsetTime = new TimeComponents(solarTime.sunset).utcDate(...dateComponents);
+        }


we will also need to reset the value of tomorrow as we use it below when setting tomorrowSunrise

z3bi · 2020-12-16T03:05:05Z

Adhan.d.ts

@@ -16,7 +16,7 @@ export class PrayerTimes {
 }

 export class CalculationParameters {
-  constructor(fajrAngle: number, ishaAngle: number, ishaInterval: number, methodName?: string)
+  constructor(methodName: string|undefined|null, fajrAngle: number, ishaAngle: number, ishaInterval: number, maghribAngle: number)


Great catch, thank you! We will still need to add the typing for the new PolarCircleResolution type and the accompanying variable on the CalculationParameters type.

src/PolarCircleResolution.js

z3bi · 2020-12-16T03:06:27Z

src/PolarCircleResolution.js

+    }
+    case PolarCircleResolution.aqrabBalad: {
+      const resolved = aqrabBaladResolver(coordinates, date, resolvedLatitude - (Math.sign(resolvedLatitude) * LATITUDE_VARIATION_STEP));
+      if (resolved) {


If we have the individual resolver functions have the same return values as this function then we can simplify this to be

return resolved | defaultReturn

src/PolarCircleResolution.js

z3bi · 2020-12-16T03:10:16Z

src/PolarCircleResolution.js

+    const solarTimePast = new SolarTime(pastDate, coordinates);
+    const solarTimePastTomorrow = new SolarTime(dateByAddingDays(pastDate, 1), coordinates);
+
+    if (!isValidSolarTime(solarTimePast) || !isValidSolarTime(solarTimePastTomorrow)) {


we could simplify having to do both past and future checks in one call by passing in a direction to indicate whether to add or subtract days to do the check. For example

const aqrabYaumResolver = (coordinates, date, daysAdded, direction)

then if the direction is positive we don't increment daysAdded but simply call back with the same daysAdded but with a negative direction. Then on the negative direction pass we do increment daysAdded and then swap back to positive.

This is a great enhancement!

z3bi

The final changes look good! We will need to update the README but I think that can be done in a separate PR.

korbav · 2020-12-17T07:28:52Z

The final changes look good! We will need to update the README but I think that can be done in a separate PR.

Great, I've just updated the documentation there for the commit log's sake.
We might need to also increment the package.json version parameter for npm to update the package ?

PS: I noticed the Pascal Case was used for the other parameters which are not proper names, I used Camel Case instead, for consistency I will change that if you're OK with that.

z3bi · 2020-12-19T17:41:12Z

@korbav yeah lets be consistent with our naming

korbav · 2020-12-19T18:03:22Z

@korbav yeah lets be consistent with our naming

@z3bi Great, that should be fine now!

korbav changed the title ~~[feat] Adding the option to handle polar night and midnight sun cases~~ [feat] Adding an option to handle the polar night and midnight sun cases Dec 14, 2020

z3bi reviewed Dec 14, 2020

View reviewed changes

src/PolarNightMidnightSunResolver.js Outdated Show resolved Hide resolved

z3bi reviewed Dec 14, 2020

View reviewed changes

src/CalculationParameters.js Outdated Show resolved Hide resolved

z3bi reviewed Dec 14, 2020

View reviewed changes

src/PrayerTimes.js Outdated Show resolved Hide resolved

z3bi requested changes Dec 14, 2020

View reviewed changes

korbav force-pushed the feat-midnight-sun-polar-night-management branch from fe89fb0 to f7a1a9c Compare December 14, 2020 21:54

z3bi requested changes Dec 15, 2020

View reviewed changes

korbav force-pushed the feat-midnight-sun-polar-night-management branch 2 times, most recently from 801c43b to cb3efcf Compare December 15, 2020 12:43

z3bi requested changes Dec 16, 2020

View reviewed changes

korbav force-pushed the feat-midnight-sun-polar-night-management branch 3 times, most recently from 98a3c09 to 3c1b187 Compare December 16, 2020 08:46

z3bi approved these changes Dec 17, 2020

View reviewed changes

korbav force-pushed the feat-midnight-sun-polar-night-management branch from 3c1b187 to 2fce4b7 Compare December 17, 2020 07:25

feat: adding the option to handle polar night and midnight sun cases

9a0f249

korbav force-pushed the feat-midnight-sun-polar-night-management branch from 2fce4b7 to 9a0f249 Compare December 19, 2020 18:02

z3bi merged commit c0a721a into batoulapps:master Dec 21, 2020

This was referenced Feb 24, 2021

Polar night & Midnight Sun features batoulapps/adhan-swift#45

Open

Polar night & Midnight Sun features batoulapps/adhan-kotlin#26

Open

Add polar night & midnight sun test cases batoulapps/adhan-testdata#1

Open

iamriajul mentioned this pull request Feb 25, 2021

Polar night & Midnight Sun features iamriajul/adhan-dart#10

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat] Adding an option to handle the polar night and midnight sun cases #30

[feat] Adding an option to handle the polar night and midnight sun cases #30

korbav commented Dec 14, 2020 •

edited

z3bi left a comment

z3bi commented Dec 14, 2020 •

edited

korbav commented Dec 14, 2020

z3bi commented Dec 14, 2020

korbav commented Dec 14, 2020

z3bi Dec 15, 2020

z3bi Dec 15, 2020

korbav Dec 15, 2020

z3bi Dec 16, 2020

z3bi Dec 16, 2020

z3bi Dec 16, 2020

z3bi Dec 16, 2020 •

edited

korbav Dec 16, 2020

z3bi left a comment

korbav commented Dec 17, 2020 •

edited

z3bi commented Dec 19, 2020

korbav commented Dec 19, 2020

[feat] Adding an option to handle the polar night and midnight sun cases #30

[feat] Adding an option to handle the polar night and midnight sun cases #30

Conversation

korbav commented Dec 14, 2020 • edited

z3bi left a comment

Choose a reason for hiding this comment

z3bi commented Dec 14, 2020 • edited

korbav commented Dec 14, 2020

z3bi commented Dec 14, 2020

korbav commented Dec 14, 2020

z3bi Dec 15, 2020

Choose a reason for hiding this comment

z3bi Dec 15, 2020

Choose a reason for hiding this comment

korbav Dec 15, 2020

Choose a reason for hiding this comment

z3bi Dec 16, 2020

Choose a reason for hiding this comment

z3bi Dec 16, 2020

Choose a reason for hiding this comment

z3bi Dec 16, 2020

Choose a reason for hiding this comment

z3bi Dec 16, 2020 • edited

Choose a reason for hiding this comment

korbav Dec 16, 2020

Choose a reason for hiding this comment

z3bi left a comment

Choose a reason for hiding this comment

korbav commented Dec 17, 2020 • edited

z3bi commented Dec 19, 2020

korbav commented Dec 19, 2020

korbav commented Dec 14, 2020 •

edited

z3bi commented Dec 14, 2020 •

edited

z3bi Dec 16, 2020 •

edited

korbav commented Dec 17, 2020 •

edited