arohl / gdis

A visualization program for the display, manipulation, and analysis of isolated molecules and periodic structures
GNU General Public License v2.0
43 stars 17 forks source link

Fix error in function change_transmat #34

Closed cajfisher closed 3 years ago

cajfisher commented 3 years ago

Matrix entry update function "change_transmat" should only change one value at a time, instead of all at once, otherwise the requested matrix data will be overwritten after the first value with values already in the boxes.

ovhpa commented 3 years ago

Hello, and thank you for your suggestion.

Have you see the pull request #21 ? I wonder if the code with your modification is properly optimizing with clang? There use to be some trouble is using a pointer to represent an interger.

Maybe we should implement the suggestion in #21

Sincerely,

cajfisher commented 3 years ago

Hi!

I don't have access to a Mac at the moment, but it should compile okay. The trick is to use GINT_TO_POINTER to send i, which should keep the compiler happy.

In pull request #21: "While this is not equivalent I think we can afford to loose the little time needed to update 12 text fields instead of 1, and gain clang optimization."

Unfortunately this will give you the wrong matrix values, as changing matrix entries greater than i before the new matrix values are displayed results in the matrix values being overwritten.

The pull request should make it work smoothly now.

Regards,

Craig


差出人: Okadome Valencia @.> 送信日時: 2021年5月20日 14:31 宛先: arohl/gdis @.> CC: クレイグ・フィッシャー @.>; Author @.> 件名: Re: [arohl/gdis] Fix error in function change_transmat (#34)

Hello, and thank you for your suggestion.

Have you see the pull request #21https://github.com/arohl/gdis/pull/21 ? I wonder if the code with your modification is properly optimizing with clang? There use to be some trouble is using a pointer to represent an interger.

Maybe we should implement the suggestion in #21https://github.com/arohl/gdis/pull/21

Sincerely,

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/arohl/gdis/pull/34#issuecomment-844710187, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AP5N32AZEUYXTPNANCFLMJ3TOSNERANCNFSM45GCKWFA.

ovhpa commented 3 years ago

Hello,

Please give me a moment to test the change with clang and I will merge the PR ;) clang was giving this problem both on Mac and Linux BTW. It was quite a challenge to find the culprit at the time, this is why I am cautious ;)

Best,

ovhpa commented 3 years ago

Hello again and sorry for the wait...

It seems that the latest version of clang does not complain with your change. Oddly enough, it also no longer complains with the original version of change_transmat before PR #21. I'm gonna merge your PR, but I hope some version / machine combination will not break again (on ARM, clang already complained that gpointer aka void * is taken from smaller integer type gint aka int).

Maybe when I have the time I will implement the small array solution, which does not cast an integer from a pointer.

Anyway, thank you for pointing that out, and thank you for your contribution.

Best,


for future references: https://developer.gnome.org/glib/stable/glib-Type-Conversion-Macros.html#GINT-TO-POINTER:CAPS #define GINT_TO_POINTER(i) ((gpointer) (glong) (i))

Original call to change_transmat function: GTK_SIGNAL_FUNC(change_transmat), (gpointer) i); Now: GTK_SIGNAL_FUNC(change_transmat), GINT_TO_POINTER(i));

cajfisher commented 3 years ago

Hi, again.

After reading your comments it dawned on me that the more robust/compiler-friendly way is to give the variable in the function header as a gpointer, not gint; the gpointer can then be converted to gint in the function. The same goes for "apply_transmat".

Sorry for messing you around like this,

C.


差出人: Okadome Valencia @.> 送信日時: 2021年5月20日 16:00 宛先: arohl/gdis @.> CC: クレイグ・フィッシャー @.>; Author @.> 件名: Re: [arohl/gdis] Fix error in function change_transmat (#34)

Merged #34https://github.com/arohl/gdis/pull/34 into master.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/arohl/gdis/pull/34#event-4771035668, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AP5N32BED2VAJISE7YIJKMTTOSXRTANCNFSM45GCKWFA.