search

cjlin1 / libsvm

LIBSVM -- A Library for Support Vector Machines
https://www.csie.ntu.edu.tw/~cjlin/libsvm/
BSD 3-Clause "New" or "Revised" License
4.54k stars 1.64k forks source link

Reduce the code differences between C and Java versions #148

Closed kno10 closed 3 years ago

kno10 commented 5 years ago

This patch reduces some trivial differences between the C and Java versions:

this should make it easier in the future to keep the C and Java versions in sync.

cjlin1 commented 5 years ago

Thanks. We will check this though it will take some time.

On 2019-08-19 07:04, Erich Schubert wrote:

This patch reduces some trivial differences between the C and Java versions:

  • add some comments missing in the Java version
  • align some variable names to match the C version
  • import static Math.* to be able to use "exp" instead of "Math.exp"
  • import static libsvm.svm_params.* to get the constants such as C_SVC as in C
  • use NULL instead of null in m4

this should make it easier in the future to keep the C and Java versions in sync.

YOU CAN VIEW, COMMENT ON, OR MERGE THIS PULL REQUEST ONLINE AT:

https://github.com/cjlin1/libsvm/pull/148

COMMIT SUMMARY

  • Improve syncronization of Java source with C source
  • import static + m4 to reduce the diff of Java to C

FILE CHANGES

  • M java/libsvm/svm.java [1] (278)
  • M java/libsvm/svm.m4 [2] (346)
  • M java/svm_predict.java [3] (32)
  • M java/svm_scale.java [4] (21)

PATCH LINKS:

-- You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub [5], or mute the thread [6]. [ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/cjlin1/libsvm/pull/148?email_source=notifications\u0026email_token=ABI3BHWQPFJH4LQPDSALCLDQFKR7DA5CNFSM4INCBJK2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HF7ZRXA", "url": "https://github.com/cjlin1/libsvm/pull/148?email_source=notifications\u0026email_token=ABI3BHWQPFJH4LQPDSALCLDQFKR7DA5CNFSM4INCBJK2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HF7ZRXA", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]

Links:

[1] https://github.com/cjlin1/libsvm/pull/148/files#diff-0 [2] https://github.com/cjlin1/libsvm/pull/148/files#diff-1 [3] https://github.com/cjlin1/libsvm/pull/148/files#diff-2 [4] https://github.com/cjlin1/libsvm/pull/148/files#diff-3 [5] https://github.com/cjlin1/libsvm/pull/148?email_source=notifications&email_token=ABI3BHWQPFJH4LQPDSALCLDQFKR7DA5CNFSM4INCBJK2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HF7ZRXA [6] https://github.com/notifications/unsubscribe-auth/ABI3BHWQOTMG3HKUWNKGHRTQFKR7DANCNFSM4INCBJKQ

ZYQue commented 5 years ago

Hello， Thank you for your commits. Most of your modification looks nice, but we still have some suggestions and wish you can help to change:

  1. svm.java is an automatically generated file, so we have no intention to modify it.
  2. For the added comment "invalid parameter" in file svm.m4, we think it will be better to use "Unreachable" (same as another place).
  3. In file svm.m4, it's ok to follow java style and keep using "null" and "boolean".

Thanks again.

kno10 commented 5 years ago

This version of svm.java was generated from svm.m4; but since it is committed to the repository, I also committed the regenerated version.

I chose to use "NULL" and "bool" macros to minimize the delta between the C and the Java version, as this makes it easier to merge any changes from C to Java, even if of course "null" and "NULL" is a trivial delta, this still means you have to compare fewer lines.

Of course your are free to cherry pick only the changes that you like. I've broken the commits into three parts - (A) sync, (B) use static imports, (C) use NULL and bool macros. Maybe you only want to pick the first two commits, minus the derived files.

cjlin1 commented 4 years ago

Hello,

We just released a new version of libsvm, in which your first commit was incorporated.

We couldn't directly sign off your commit because of some minor problems (one or two changes in your commit 1 indeed should be in commit 2). But in our commit message we did clearly indicate your contribution.

We decided not to take the commit 2, in which packages are imported. The reason why we used Math.abs rather than imported Math package is to avoid naming conflicts. In C we cannot do this but in java we can. Thus we used specific names since the beginning. We also need backward compatibility. The change may cause some existing java code using libsvm to have problems. Thus in the end we decided not to take it.

Thank you again for the help and contribution.

kno10 commented 4 years ago

Hello, It is perfectly fine with me to adapt the commit.

Wrt. the second commit: Imports are local to the current class only (that is why you always have to import all the stuff again), and used only at compile time. So the import static java.lang.Math.* I proposed should yield exactly the same byte code as the original version: there is no "import" in the bytecode; either is expanded by the compiler to the fully qualified java.lang.Math.abs in the bytecode, and must - by the Java language specification and design - not have an effect on outside classes. Encapsulation is very strong in Java.

I have not verified if the class files produced are exactly the same; but they should, unless I again have some smaller mix-up in splitting the commits.

cjlin1 commented 4 years ago

Thanks.. My students and I will investigate this a bit more. We are a bit slow but will keep you informed. Thanks again.

On 2019-09-10 13:22, Erich Schubert wrote:

Hello, It is perfectly fine with me to adapt the commit.

Wrt. the second commit: Imports are local to the current class only (that is why you always have to import all the stuff again), and used only at compile time. So the import static java.lang.Math.* I proposed should yield exactly the same byte code as the original version: there is no "import" in the bytecode; either is expanded by the compiler to the fully qualified java.lang.Math.abs in the bytecode, and must - by the Java language specification and design - not have an effect on outside classes. Encapsulation is very strong in Java.

I have not verified if the class files produced are exactly the same; but they should, unless I again have some smaller mix-up in splitting the commits.

-- You are receiving this because you commented. Reply to this email directly, view it on GitHub [1], or mute the thread [2]. [ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/cjlin1/libsvm/pull/148?email_source=notifications\u0026email_token=ABI3BHRN3FNJZ7J7N4J4NJTQI76WRA5CNFSM4INCBJK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6MLZMQ#issuecomment-530103474", "url": "https://github.com/cjlin1/libsvm/pull/148?email_source=notifications\u0026email_token=ABI3BHRN3FNJZ7J7N4J4NJTQI76WRA5CNFSM4INCBJK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6MLZMQ#issuecomment-530103474", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]

Links:

[1] https://github.com/cjlin1/libsvm/pull/148?email_source=notifications&email_token=ABI3BHRN3FNJZ7J7N4J4NJTQI76WRA5CNFSM4INCBJK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6MLZMQ#issuecomment-530103474 [2] https://github.com/notifications/unsubscribe-auth/ABI3BHQNJOMKEIKVFBG63DLQI76WRANCNFSM4INCBJKQ

cjlin1 commented 3 years ago

We finally checked this issue again (with the help of Bill Tran) From https://docs.oracle.com/javase/tutorial/java/package/usepkgs.html they mentioned "Use static import very sparingly. ... readers of the code won't know which class defines a particular static object." So in the end we decide to be conservative and explicitly specify package+function.

Thank you again for the pull request.