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

[Pytorch, OpenCV] Implicit ArrayRef, Updates for JavaCPP 1.5.10 #1455

Merged
merged 14 commits into from
Jan 14, 2024

Conversation

HGuillemet
Copy link
Collaborator

@HGuillemet HGuillemet commented Dec 30, 2023

Pytorch

  • Have methods taking an ArrayRef accept also a Vector (relying on C++ converting constructor of ArrayRef) when the corresponding vector template instance has been mapped in Java. This allows to write for instance:
    Tensor tensor2 = tensor1.index(new TensorIndexVector(new TensorIndex(x), new TensorIndex(new Slice())));
    instead of explicitly constructing the TensorArrayRef to pass to index. This is still much heavier that what we can write in C++:
    Tensor tensor2 = tensor1.index({x, Slice()});
    but it's better than with explicit use of ArrayRef.
  • Adjust infos for templated constructors (PR Improve parsing of operator and function templates javacpp#732)
  • Adjust infos for template specializations (PR Fix parsing of template specializations and templated friend functions javacpp#733)

OpenCV

  • Add Info.upcast() on Algorithm to prevent crashes when upcasting or downcasting
  • Add template instantiations for cv::read and cv::write

@HGuillemet HGuillemet changed the title [Pytorch] WIP [Pytorch, OpenCV] Implicit ArrayRef, Updates for JavaCPP 1.5.10 Jan 9, 2024
@saudet
Copy link
Member

saudet commented Jan 10, 2024

Is this pull request done? You're not going to add anything more here for other presets?

@HGuillemet
Copy link
Collaborator Author

Done

@saudet
Copy link
Member

saudet commented Jan 13, 2024

Please don't push 200 times a day, Google Drive really doesn't like it, and I don't know how to fix this

@HGuillemet
Copy link
Collaborator Author

I haven't pushed anything for 3 days. Can I help somehow ?

@saudet
Copy link
Member

saudet commented Jan 13, 2024 via email

@HGuillemet
Copy link
Collaborator Author

According to the log above, there was just 3 pushes the same day, 4 days ago. No particular reason. I just worked this whole day on the PR I guess.
Is the problem due to these 3 pushes a day or that the third one was a forced-push ?

@saudet
Copy link
Member

saudet commented Jan 13, 2024

I asked you why do you had to push more than once a day, and you basically answered that you don't need to. So, don't push more than once a day! Ok? Whenever you push something, the builds for everything that you touched get triggered. Don't do that. Maybe I should just disable automatic triggers for pull request if that feature exists (it doesn't seem to exist), but jeez, just don't push more than once a day! Just don't do it

@saudet saudet merged commit 9697dcf into bytedeco:master Jan 14, 2024
43 checks passed
@HGuillemet HGuillemet deleted the hg_pytorch branch January 14, 2024 11:19
@saudet
Copy link
Member

saudet commented Jan 27, 2024

@HGuillemet The SimpleMNIST sample code doesn't work anymore after merging this. Please fix it

java.lang.NullPointerException: Pointer address of argument 0 is NULL.
    at org.bytedeco.pytorch.MNISTBatchDataset.map (Native Method)
    at SimpleMNIST.main (SimpleMNIST.java:82)
    at org.codehaus.mojo.exec.ExecJavaMojo.lambda$execute$0 (ExecJavaMojo.java:283)
    at java.lang.Thread.run (Thread.java:840)

@HGuillemet
Copy link
Collaborator Author

This is the issue about Stack mentioned in bytedeco/javacpp#733.
To sum it up:
Stack primary template is a declaration without body.
Template specializations (that have bodies in this case), are now ignored.
So the Java classes mapping template instances are @Opaque with no constructor and new ExampleStack() used in SimpleMNIST returns an object with a null address.

I'll add an ugly javaText to give the text of the whole class and force the generation of a constructor.
Unless you have a better idea ?

It could be nice to be able in Info to specify an arbitrary javaText to be appended to a class without having to give the text of the whole class or to hook to another existing, unrelated, declaration in the class.

Shall I push the commit to this PR even if it has already been merged ? or open another ?

@saudet
Copy link
Member

saudet commented Jan 28, 2024

I'll add an ugly javaText to give the text of the whole class and force the generation of a constructor. Unless you have a better idea ?

We can just replace the constructor, no?

It could be nice to be able in Info to specify an arbitrary javaText to be appended to a class without having to give the text of the whole class or to hook to another existing, unrelated, declaration in the class.

Yeah, I thought I did that somewhere already, but I guess not. It's not obvious how it would work. We'd probably need to add new flags and what not, so unless we figure out a nice way to do it without that, let's not do that

Shall I push the commit to this PR even if it has already been merged ? or open another ?

Another pull request please, but I'm pretty much done with the release I think, so if no one complained until now I guess no one needs it, so we can do that later I suppose, sounds good?

@HGuillemet
Copy link
Collaborator Author

I'll add an ugly javaText to give the text of the whole class and force the generation of a constructor. Unless you have a better idea ?

Well, in fact I don't know how to replace the whole class text. If I add a javaText to the type, it's copied elsewhere when the type is used, not only in the class definition.

We can just replace the constructor, no?

The constructor is generated here.
How would you hook an info to target the constructor ?

Another pull request please, but I'm pretty much done with the release I think, so if no one complained until now I guess no one needs it, so we can do that later I suppose, sounds good?

As you wish. No idea if ExampleStack and TensorExampleStack is of common use. I don't use the dataset api myself.
See also the JavaCPP issue 740 I posted some minutes ago and if you want to include something about it before the release.

@saudet
Copy link
Member

saudet commented Jan 29, 2024

No, I'm not going to wait after you fixing things endlessly. You keep breaking things, so if someone complains, please fix it. Otherwise, it's not important I suppose.

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

Successfully merging this pull request may close these issues.

2 participants