Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rounding behavior option #43

Closed
z3bi opened this issue Feb 27, 2021 · 22 comments
Closed

Add rounding behavior option #43

z3bi opened this issue Feb 27, 2021 · 22 comments
Labels

Comments

@z3bi
Copy link
Contributor

z3bi commented Feb 27, 2021

based on batoulapps/adhan-swift#43

@farisfaisalthena
Copy link

Hi just curious any idea on when this would be implemented? This is because I am waiting to implement them on my system and currently not able to due to inaccurate timings

@z3bi
Copy link
Contributor Author

z3bi commented Jun 14, 2021

@farisfaisalthena this feature is fairly minor, since its just the rounding at the very end it shouldn't affect times by more than 1 minute. At the moment the only method that implements a different rounding is the Singapore/JAKIM method. What inaccurate timings are you seeing that is preventing you from using this library?

@farisfaisalthena
Copy link

The right is from your library and left is JAKIM. Its just not big difference but its better to get the accurate time since users rely on apps for the prayers time

time

@z3bi
Copy link
Contributor Author

z3bi commented Jun 14, 2021

The biggest reason for differences in times is that JAKIM uses a singular coordinate for different regions, you can see those regions and coordinates here https://www.e-solat.gov.my/portalassets/files/Lampiran%20B.pdf

Are you comparing using the exact region coordinates specified in the above document?

@farisfaisalthena
Copy link

Yes

@z3bi
Copy link
Contributor Author

z3bi commented Jun 14, 2021

@farisfaisalthena can you post the coordinates/zone you're using and all the params you used to get these times?

@farisfaisalthena
Copy link

Latitude: 3.2508990079879156 & Longitude: 101.6883735099779

And its Gombak Zone

@farisfaisalthena
Copy link

const coordinates = new adhan.Coordinates(res.coords.latitude, res.coords.longitude);
const date = new Date();
const params = adhan.CalculationMethod.Singapore();

  this.time = new adhan.PrayerTimes(coordinates, date, params);

@z3bi
Copy link
Contributor Author

z3bi commented Jun 14, 2021

So in the document, the "Gombak Zone" lists the following coordinates:

U3° 44’
T101° 23’

translated into decimal would give you new adhan.Coordinates(3.7333333333, 101.3833333333)

if you use those coordinates you will find the times are now almost exact, and that there is only a difference of 1 minute that should be resolved by the rounding changes.

Again, the main way to achieve the same times as the JAKIM table is to use the EXACT coordinates listed in their document. I would recommend in your code making a list of those exact coordinates and then finding the closest matching one for the user's given coordinates.

The rounding change will help, but a difference of 1 minute is a small enough variance that it is still acceptable to proceed with for now.

@farisfaisalthena
Copy link

Actually the coordinates are not entered manually since it's being used in Malaysia and Indonesia, the coordinates are taken automatically based on where the users currently are. This is because having it hard coded is not an efficient way for users

@z3bi
Copy link
Contributor Author

z3bi commented Jun 14, 2021

@farisfaisalthena right so if you want to match JAKIM website exactly, you need to find the closest JAKIM coordinates from the user's coordinates (my suggestion is to make a list of the known JAKIM coordinates and then iterating through and using distance algorithm to find the closest one).

OR

if you're not going to match the coordinates exactly, then you should be ok with the times being different by a few minutes. Again, these times aren't wrong they're just for more specific coordinates.

@farisfaisalthena
Copy link

if you're not going to match the coordinates exactly, then you should be ok with the times being different by a few minutes. Again, these times aren't wrong they're just for more specific coordinates.

Probably will try this method but would be nice if the library handles that based on the coordinates given it will detect the nearest and display. Also is there a link where i can refer for indonesia coordinates like the JAKIM one u provided? Also I would need it to get exactly the time because, users will complain with not being exact. This issue happened before which is why i needed it to be exact

@korbav
Copy link

korbav commented Jun 15, 2021

Salam alaykoum, I'm bringing my 2 cents if you don't mind.
First of all, I totally agree with @z3bi on the fact that the given times are actually, more precise and then certainly not wrong.
Though, I also agree that, IMHO, what eventually matters, especially when you pick a pre-designed calculation method, which is the case here with adhan.CalculationMethod.Singapore, we should ideally handle as much as possible the specificities of this given method in order to be sync with what is actually used by the different supports/organizations in this region.
It might be impossible to reach this level of granularity for all the different methods, but I feel like, in the case described here, we could quite easily.

I created a sandbox that could easily be translated to a PR if ever needed : https://codesandbox.io/s/wild-voice-xo7fd?file=/src/index.js:6111-6144

I basically converted manually all the DPS coordinates to decimal ones, grouped them into a list, then browsed this list to find the closest well-known position to the user's position and used it for the calculation.

The algorithm works, though, I tried to obtain the same timing table published above and still noticed differences, I'm not sure if this table stands for the date of June 14th 2021, but if it's the case, it means that there must be other parameters to tweak.

@z3bi
Copy link
Contributor Author

z3bi commented Jun 15, 2021

@korbav thank you for the sandbox. This is an interesting case though. Implementing this functionality in the library would require the developer to know that
a) the user's coordinates are located in Malaysia
b) that they are more interested in the published JAKIM times rather than the true timing of the sun

I don't think theres any safe way to apply this logic automatically, but perhaps an argument could be made to add an optional utility function for this purpose.

@z3bi
Copy link
Contributor Author

z3bi commented Jun 15, 2021

Also is there a link where i can refer for indonesia coordinates like the JAKIM one u provided?

@farisfaisalthena I don't have a link, I didn't even know that's what Malaysia was doing until I stumbled upon that document by accident. I think it's worth looking for that document, please let me know if you have any luck finding it.

@z3bi
Copy link
Contributor Author

z3bi commented Jun 15, 2021

@farisfaisalthena I did some investigation and found the official indonesian prayer times here

https://bimasislam.kemenag.go.id/jadwalshalat

if you inspect the JSON that comes back when you generate the prayer times it contains the coordinates used.

{"status":1,"message":"Success","prov":"DKI JAKARTA","kabko":"KOTA JAKARTA","lintang":"6\u00b0 10' 12.32\" S","bujur":"106\u00b0 49' 51.78\" E","data":{"2021-06-01":{"tanggal":"Selasa, 01\/06\/2021","imsak":"04:27","subuh":"04:37","terbit":"05:54","dhuha":"06:23","dzuhur":"11:54","ashar":"15:15","maghrib":"17:47","isya":"19:01"},...

it seems feasible to scrape that data, if anyone is interested in gathering that data I think at the very least providing the table of coordinates would be something we can do in this library.

@korbav
Copy link

korbav commented Jun 15, 2021

@korbav thank you for the sandbox. This is an interesting case though. Implementing this functionality in the library would require the developer to know that
a) the user's coordinates are located in Malaysia
b) that they are more interested in the published JAKIM times rather than the true timing of the sun

I don't think theres any safe way to apply this logic automatically, but perhaps an argument could be made to add an optional utility function for this purpose.

@z3bi Thanks for the feedback.
In my opinion, the most straightforward way to tackle this would be to create a tweaked calculation method.

2 cases would be to consider depending on how JAKIM works :

  • If the zones are shared between different countries (Malaysia, Indonesia), then we should have an adhan.CalculationMethod.JAKIM calculation method.

  • If the zones are not shared and are specific to the corresponding countries (Malaysia, Indonesia), then we should have 2 different methods calculation method :

    • adhan.CalculationMethod.Malaysia_JAKIM
    • adhan.CalculationMethod.Indonesia_JAKIM

This approach would have the advantage to eliminate the concern of determining the user's country, and would also have the advantage to give an explicit calculation choice to the user.

@z3bi
Copy link
Contributor Author

z3bi commented Jun 15, 2021

@korbav the calculation method and the coordinates are two different parameters that get passed into PrayerTimes. Your suggestion would mean doing the lookup inside the PrayerTimes. I'm not a fan of this behavior happening there as I think it breaks the contract that you're getting prayer times for the coordinates that you passed in, and no other method has the side effect of changing the coordinates. It would make more sense for the library to have a utility function on the Coordinates object

myCoordinates.lookupJakimCoordinates(Coordinates.JakimTable.Malaysia) and myCoordinates.lookupJakimCoordinates(Coordinates.JakimTable.Indonesia)

the developer would then pass those coordinates into PrayerTimes.

From a developer perspective it's more obvious what is happening to your parameters, and from an end user perspective you could present them with the same choices (JAKIM Malaysia and JAKIM Indonesia) and then simply pass the coordinates through the lookup functions when they select those choices.

@korbav
Copy link

korbav commented Jun 15, 2021

@korbav the calculation method and the coordinates are two different parameters that get passed into PrayerTimes. Your suggestion would mean doing the lookup inside the PrayerTimes. I'm not a fan of this behavior happening there as I think it breaks the contract that you're getting prayer times for the coordinates that you passed in, and no other method has the side effect of changing the coordinates. It would make more sense for the library to have a utility function on the Coordinates object

myCoordinates.lookupJakimCoordinates(Coordinates.JakimTable.Malaysia) and myCoordinates.lookupJakimCoordinates(Coordinates.JakimTable.Indonesia)

the developer would then pass those coordinates into PrayerTimes.

From a developer perspective it's more obvious what is happening to your parameters, and from an end user perspective you could present them with the same choices (JAKIM Malaysia and JAKIM Indonesia) and then simply pass the coordinates through the lookup functions when they select those choices.

@z3bi I got your point, but if my understanding is right, the "Jakim method" consists of doing this lookup 100% of the time, so I'm not sure to get why would it break any contract? A user who would choose this method would explicitly & implicitly trust it.

If there's any need, and I doubt there is any, to be honest, the developer can still remind the user that this method will alter hisgiven coordinates. Personally, if I decide to choose a pre-defined method, it means that whatever would happen behind the scene, I'm expecting to find the same timetable that I would find anywhere else.

@z3bi
Copy link
Contributor Author

z3bi commented Jun 15, 2021

@korbav so I think theres nothing inherently wrong with using the JAKIM method for a user in Vietnam for example. Making a change like this goes against how the library inherently allows any method to be applied to any location. I'm in favor of leaving that flexibility by making this an optional pre-step that a user can take.

@korbav
Copy link

korbav commented Jun 15, 2021

@korbav so I think theres nothing inherently wrong with using the JAKIM method for a user in Vietnam for example. Making a change like this goes against how the library inherently allows any method to be applied to any location. I'm in favor of leaving that flexibility by making this an optional pre-step that a user can take.

I see, if there's a possibility to use this method anywhere then there's clearly no point in doing anything library-side.
Let's keep the library as it is.
@farisfaisalthena You can hopefully refer to my sandbox to implement your solution

@z3bi z3bi closed this as completed in 7cf40d4 Jun 17, 2021
z3bi pushed a commit that referenced this issue Jun 17, 2021
# [4.2.0](v4.1.0...v4.2.0) (2021-06-17)

### Features

* **calculation-parameters:** adding rounding option parameter ([7cf40d4](7cf40d4)), closes [#43](#43)
* **date-utils:** default rounding logic to nearest ([87f2b99](87f2b99))
* **date-utils:** include rounding changes in webpacked artifact ([fa8a13e](fa8a13e))
@github-actions
Copy link

🎉 This issue has been resolved in version 4.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants