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

Increase inheritable objects #174

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

BauerMarco
Copy link

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.

@maxscheurer
Copy link
Member

If you rebase this against the current master branch, the pipeline should (hopefully) run.

@BauerMarco
Copy link
Author

Done

@BauerMarco
Copy link
Author

Now the clang-formatter should be happy too

@maxscheurer
Copy link
Member

I still don't fully understand the purpose of this PR.

What do you mean by "not accessible" from the adcc module?

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., validate_state_parameters via adcc/__init__.py.

Am I missing something?

@BauerMarco
Copy link
Author

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 adcc/__init__, but if you don't agree, I can reduce it to the change in libadcc.

@@ -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());
Copy link
Member

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:

  1. Creating a new, possibly dangling AdcMemory instance here. AFAIK, this is usually only created once and used throughout.
  2. Importing libtensor directly in the pybind export file. Usually, we only import libtensor in TensorImpl.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.
@BauerMarco
Copy link
Author

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.

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