Improvement to the `qiskit_nature.second_q.mappers` module

[mrossinek]

What should we add?

A few things in the mappers module could be improved. I will try to summarize some thoughts below based on gathered observations. (See also this thread: https://github.com/qiskit-community/qiskit-nature/pull/1270#discussion_r1408833996.)

• [ ] the QubitMapper.mode_based_mapping method should really not be part of the base class. Instead, we could implement this as a ModeBasedMapper class which only the mappers that actually follow this implementation derive from (i.e. JordanWignerMapper, ParityMapper, BravyiKitaevMapper, and DirectMapper). This would avoid all other mappers to have this public method without it actually working (since they don't implement the pauli_table classmethod.

• [ ] we should revert the use of classmethod for the pauli_table in those mappers above, because this broke the initial implementation of the Bonsai mapper which was done externally. We should see if we can still keep the caching that was added in #699 as part of this change, to avoid a performance regression (#644).

I think the above two tasks could be combined into a single PR quite nicely.

• [ ] Performance: Rather than duplicating #771 I am simply referring to it here for completeness.

[grossardt]

Re: ModeBasedMapper

What would you do with the FermionicMapper/BosonicMapper classes that are a layer in between right now? I guess something like JordanWignerMapper would have to inherit from both FermionicMapper and ModeBasedMapper then?

Re: pauli_table

So pauli_table should be an instance method, do I get that right? Regarding the caching, I think one could simply do that on the level of the actual implementation of pauli_table by having a helper classmethod or staticmethod _pauli_table with @lru_cache decorator.

I need to do something similar for the TernaryTreeMapper which has an attribute for the priority of X, Y, Z and the return value of pauli_table depends on this attribute. So I simply introduced a staticmethod _pauli_table that takes one extra argument and wrap lru_cache around that instead of the original pauli_table classmethod. Although that works with a class attribute, it would make a lot more sense to implement pauli_table as an instance method also for the TernaryTreeMapper and then have the priority be a regular attribute of an instance.

[mrossinek]

Re: ModeBasedMapper

What would you do with the FermionicMapper/BosonicMapper classes that are a layer in between right now? I guess something like JordanWignerMapper would have to inherit from both FermionicMapper and ModeBasedMapper then?

Yes, they would subclass both :+1:

Re: pauli_table

So pauli_table should be an instance method, do I get that right? Regarding the caching, I think one could simply do that on the level of the actual implementation of pauli_table by having a helper classmethod or staticmethod _pauli_table with @lru_cache decorator.

Yes, an instancemethod would be preferable. In case we want to parallelize these methods, we will need to be careful when using lru_cache and mutable input arguments (such as lists) because these will have non-matching IDs across threads (just FYI).

[grossardt]

I would offer to implement the first two things. I finished the Ternary Tree Mapper and wanted to start a PR for that today-ish, but I could include these changes and the new mapper into a single PR if that would be okay? Otherwise, I guess it is a bit pointless to start a PR for the new mapper now just to need to restructure it again once these changes are done.

Regarding the caching:

• Thanks for pointing that out with the mutable inputs. Shouldn't be an issue because input will be string (I add a pauli_priority argument that can be one of 'XYZ', 'XZY', ...) but good to keep in mind.
• I have little experience with caching, but my feeling is that decision whether to cache should be made on an individual level at the concrete implementation? I would, therefore, get rid of all the @lru_cache decorators for the abstract methods in QubitMapper?
• Specifically, I needed to remove the decorator from QubitMapper.sparse_pauli_operators because it messed up my implementation of the Ternary Tree Mapper. My preferred solution would be to have these methods call some private method, e.g. ._sparse_pauli_operators, and then use @lru_cache decorators on those where it makes sense. Then those can also be implemented as static methods or instance methods as needed. What do you think?

[mrossinek]

I would offer to implement the first two things. I finished the Ternary Tree Mapper and wanted to start a PR for that today-ish, but I could include these changes and the new mapper into a single PR if that would be okay? Otherwise, I guess it is a bit pointless to start a PR for the new mapper now just to need to restructure it again once these changes are done.

If you want to work on this, please go ahead, by all means! I suggest to keep these in separate PRs, just to keep reviewing simpler. Especially, because some of these changes may need to propagate through all classes. Just keep in mind, that reviewing might slow down for the rest of year because many of us will be on vacation over the upcoming holidays.

• I have little experience with caching, but my feeling is that decision whether to cache should be made on an individual level at the concrete implementation? I would, therefore, get rid of all the @lru_cache decorators for the abstract methods in QubitMapper?

Yes, I think that is a good call. I think, as things are now, the caching actually added little value.

• Specifically, I needed to remove the decorator from QubitMapper.sparse_pauli_operators because it messed up my implementation of the Ternary Tree Mapper. My preferred solution would be to have these methods call some private method, e.g. ._sparse_pauli_operators, and then use @lru_cache decorators on those where it makes sense. Then those can also be implemented as static methods or instance methods as needed. What do you think?

How about you do a first PR which does only API cleanup:

• [ ] extract the ModeBasedMapper
• [ ] remove all caching (for now)
• [ ] make pauli_table an instance method

If you can, doing some small performance tests for before and after would be amazing. See for example the following old issues which discussed performance:

• https://github.com/qiskit-community/qiskit-nature/issues/545
• https://github.com/qiskit-community/qiskit-nature/issues/644
• https://github.com/qiskit-community/qiskit-nature/issues/1129

[grossardt]

On a side note: I noticed that the base classes FermionicMapper and BosonicMapper are not included in the __init__.py (and thus also not in the apidocs). I am unsure whether this is on purpose but given that other base classes like e.g. QubitMapper or SparseLabelOp are included, I guess it should be changed?

Apart from that, will do as suggested (maybe this weekend).

[mrossinek]

On a side note: I noticed that the base classes FermionicMapper and BosonicMapper are not included in the __init__.py (and thus also not in the apidocs). I am unsure whether this is on purpose but given that other base classes like e.g. QubitMapper or SparseLabelOp are included, I guess it should be changed?

This was a conscious decision as part of https://github.com/qiskit-community/qiskit-nature/pull/1044. See its discussion for some more insights.

Apart from that, will do as suggested (maybe this weekend).

Awesome! I will likely only be able to review your PR in the new year (just to make you aware).

[grossardt]

On a side note: I noticed that the base classes FermionicMapper and BosonicMapper are not included in the __init__.py (and thus also not in the apidocs). I am unsure whether this is on purpose but given that other base classes like e.g. QubitMapper or SparseLabelOp are included, I guess it should be changed?

This was a conscious decision as part of #1044. See its discussion for some more insights.

I see. I won't touch that then.

Awesome! I will likely only be able to review your PR in the new year (just to make you aware).

No problem at all. Given that Christmas approaches - as every year - surprisingly fast, chances are that I won't even open a PR before then ;).