rleap-project/dlplan

Construct state from atom indices

Closed this issue · 4 comments

The State constructor currently accepts a vector of Atoms and then converts them to atom indices. I think it would be good to extend the API so that it's possible to directly pass the atom indices. This would allow me to avoid converting from indices to atoms and then back to indices (since I'm storing the indices for efficiency).

I see that you had a similar API in the past. Let me suggest making some slight changes:

StateImpl::StateImpl(std::shared_ptr<const InstanceInfo> &instance_info, Index_Vec &&atom_idxs)
    : m_instance_info(instance_info), m_atom_idxs(convert_atoms(*instance_info, move(atom_idxs))) { }

static Index_Vec convert_atoms(const InstanceInfo& instance_info, Index_Vec &&atom_idxs) {
    assert(std::all_of(atom_idxs.begin(), atom_idxs.end(), [&](int atom_idx){return utils::in_bounds(atom_idx, instance_info.get_atoms());}));
    atom_idxs.reserve(atom_idxs.size() + instance_info.get_static_atom_idxs().size());
    atom_idxs.insert(atom_indices.end(), instance_info.get_static_atom_idxs().begin(), instance_info.get_static_atom_idxs().end());
    return move(atom_idxs);
}

Are you already looking into this, or would it help if I created a PR?

I haven't because my expectation is that is does not give a lot of improvement because this is just 1 vector allocation compared to many more allocations during evaluation. However, I will do that next because it is very easy to do.

done :)

Awesome, thanks!