-
Notifications
You must be signed in to change notification settings - Fork 21
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
Increase inheritable objects #174
base: master
Are you sure you want to change the base?
Increase inheritable objects #174
Conversation
If you rebase this against the current |
Done |
Now the clang-formatter should be happy too |
I still don't fully understand the purpose of this PR. What do you mean by "not accessible" from the In your project, you could just import from sub-modules directly, e.g., from adcc.workflow import validate_state_parameters rather than exposing, e.g., Am I missing something? |
This is true. The important part in this PR is the cpp part, where I introduced the new function set_lt_scalar, which is something I cannot hack together with a few lines of code outside of adcc. Since I needed the modules for the external package, I thought they should be exposed through |
libadcc/pyiface/export_Tensor.cc
Outdated
@@ -346,6 +349,17 @@ static py::object Tensor___repr__(const Tensor& self) { | |||
return Tensor___str__(self); | |||
} | |||
|
|||
static ten_ptr set_lt_scalar(const py::float_ n) { | |||
auto adcmem_ptr = std::shared_ptr<AdcMemory>(new AdcMemory()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit worried about the following:
- Creating a new, possibly dangling
AdcMemory
instance here. AFAIK, this is usually only created once and used throughout. - Importing
libtensor
directly in the pybind export file. Usually, we only importlibtensor
inTensorImpl.cc
.
If you have a second, @mfherbst, what do you think about it?
If I understand correctly, this functionality just enables using a single number as item in an amplitude vector. With some boilerplate, this could possibly very easily achieved on the Python level, right?
…r function is now used. Furthermore, instead of just a scalar an arbitrary 1d array can be initialized. Using and mathematical operations like arbitrary libtensors can now be initialized and set from the python side. This doesn't support symmetry though.
I've addressed your points in the new commit and also extended the initialization from a single scalar to a 1d array. I've searched through everything, which is imported to the python side from libadcc, but could't find anything to initialize libtensor arrays of arbitrary dimension and length from the python side. If there is anything, I've missed it. |
I rewrote pull request #153 as a separate repository called polaritonic_adcc on my home github, where I noticed that some functionalities I needed were not accessible from the adcc module. Before I simply copy, paste and reference a lot of the adcc code, it would be much easier if this pull request got accepted, in case somebody else also wants to build a similar project.
I also had to add an additional function to the libadcc site, which lets you set a libtensor array with one entry from the python side. I didn't really see an other option, but if you guys have a good idea for a workaround, I could remove it from this pull request.